Strange behaviour on consecutive .fromJS() calls

[lisandropuzzolo]

On some cases, making consecutive calls to ko.mapping.fromJS(....) using "create definitions", makes unusable the computables created by those "creates" (its values gets fixed at the initial value, and doesn't compute anymore).

I can't find the reason of this behavior, but I've found a fix:

Please insert this code before the following line: exports.fromJS = function (jsObject /, inputOptions, target/ ) {

    exports.evaluateDependentObservables = function(){
        while (dependentObservables.length) {
            var DO = dependentObservables.pop();
            if (DO) DO();
        }
    }

Then, if I call ko.mapping.evaluateDependentObservables() between each ko.mapping.fromJS everything works as expected.

[sagacity]

This call should not be needed. Can you make a reproduction scenario on jsfiddle that illustrates the problem?

[lisandropuzzolo]

Hi Roy. I couldn't reproduce this issue on a simple viewmodel, that's why I didn't fiddled it. But I will try to find some simple case where this issue appears.

It seems that since the evaluation of the observables occurs on a timeout, the observables / computed from the first ko.mapping.fromJS call are evaluated AFTER the second ko.mapping.fromJS call, and that is what is making the problem.

Again, I hope I will be able to find a simple example to post here, since my actual model is quite big.

[sagacity]

Okay, please update this issue when you were able to repro and I'll take a look.

[lisandropuzzolo]

It took me some time to find a way to isolate the problem but here it is. I've found it using ko.validation plugin but It also occurs without ko.validation so I wrote a fiddle that does't use ko.validation but does something similar:

first fiddle

http://jsfiddle.net/lisandropuzzolo/VCP4t/

This fiddle should write that user.isValid() is false, but as you can see, it output true first, and false after a setTimeout.

vm = {
    submodel: {
        user: ko.observable('')
    }
}

The 'user' property becomes an observable after applying the mapping, and then I extend it with a custom extender {required:true} which adds to it a computed (isValid) that tells if the value is not an empty string.

vm.submodel.user.isValid(); // should output false since its value is always an empty string

But what happens on the ViewModel constructor is that user.isValid() tells it is valid even when its value is always an empty string. After a setTimeout, user.isValid() tells its's false.

This happens because the computed h_obsValidationTrigger is NOT being evaluated since mapping defer its evaluations.

---

seccond fiddle

But it gets worse if I run another ko.mapping.fromJS right after the first one. This time, user.isValid tells it's true even after the setTimeout (just a little change on the last line, passing true to the constructor so it calls ko.mapping.fromJS one more time):

Here is the other fiddle that shows it: http://jsfiddle.net/lisandropuzzolo/vGzBm/

---

¿possible solution?

The only solution I've found so far, is to modify knockout.mapping.js and add this function after exports.fromJS:

exports.evaluateDependentObservables = function(){
    while (dependentObservables.length) {
        var DO = dependentObservables.pop();
        if (DO) DO();
    }
}

and calling evaluateDependendObservables() after the seccond ko.mapping.fromJS() call.

This way, the user.isValid() is evaluated, gets it's right computed value, and I can make another ko.mapping.fromJS() call.

¿Is there anyway to fix this without calling evaluateDependentObservables()?

Thanks for your time.

[drdamour]

i'm having the exact same problem with KO Validation + KO.Mapping. I've traced the problem to the deferring of evaluating the "proxied" via setTimeout. This makes it happen after the current code thread exits which is why subsequent mappings are getting screwed up.

I changed this:

if (!--mappingNesting) {
    window.setTimeout(function () {
        while (dependentObservables.length) {
            var DO = dependentObservables.pop();
            if (DO) DO();
        }
    }, 0);
}

to this:

if (!--mappingNesting) {
    while (dependentObservables.length) {
        var DO = dependentObservables.pop();
        if (DO) DO();
    }
}

i'm fuzy on the reason the execution is delayed till after everything happens, doesn't it just need to happen after all other observables have been created for the current model? What is the setTimeout buying us?

[drdamour]

here's the commit that added it https://github.com/SteveSanderson/knockout.mapping/commit/9a38c5d4922277bb7d4534aa3a79d2429c1d1f23

[drdamour]

so i removed the setTimeout call and just did it at the end, 3 tests / 4 assertions failed (really 6 / 8 cause the way the tests are run twice based upon computed or observable). I think none of them are relevant. Here's why.

1st one is easy "dependentObservable dependencies trigger subscribers" @ https://github.com/SteveSanderson/knockout.mapping/blob/master/spec/proxyDependentObservableBehaviors.js#L322. This test is setup for async..but we aren't async anymore. the first evaluationCount should be 1 because it should have been set by the time fromJS returns. Simple logically change and it passes.

2nd one was a pain because the test is completely invalid. "can subscribe to nested proxy dependentObservable" @ https://github.com/SteveSanderson/knockout.mapping/blob/master/spec/proxyDependentObservableBehaviors.js#L278 This one is invalid because it's depending on a behaviour that KO itself doesn't have. in the test, when the computed writable observable is created a subscription to it is made and it tests to make sure that the writes apply and the subscription launches. Computed writables don't work this way for either condition. if you write to then read from a writable computed, your write better set an observable your read is based upon, if it doesn't then it will think it's already computed and will not fire anything subscribed to it. Check this fiddle: http://jsfiddle.net/drdamour/9Pz4m/ and you'll see that this test is invalid as it's not mimicing the behaviour of computed writeable observables sans mapping. A valid test would be to make the read and write work with an observable on the generated ViewModel.

3rd to fail is "nested calls to mapping should not revert proxyDependentObservable multiple times". https://github.com/SteveSanderson/knockout.mapping/blob/master/spec/proxyDependentObservableBehaviors.js#L177 this is invalid for the same reason as #2. It's relying on some behaviour where a computed observable isn't based on an observable, thus if the value underneath changes the computed won't know and won't update itself. This is how KO works, this test is ONLY verifying that the behaviour is strange. Further it to this:

var vm = ko.mapping.fromJS(vmjs, mapping);
equal(vm.inner1.DOprop(), vm);
var vm2 = vm;
vm = "some new value";
equal(vm2.inner1.DOprop(), vm);

you'd expect that test to fail, right? also in this test "var vm" is declared twice, not sure that was intentional or not.

I'm going to do a fork & pull request with these changes to code and tests

[sagacity]

@drdamour Thanks for the analysis, I'll have to go and check what the original reasoning behind the asynchronous execution was. It's clearly not ideal (as you and others have found out) so if it can go, I'd be more than happy. I'll get back to you!

[sagacity]

Welp, it appears that I made some incorrect assumptions when writing this code (as evidenced by test 2 you're mentioning above). I'm currently looking at my company's codebase and I'm running into a few issues, which are dependent on these incorrect assumptions.

I'll need to investigate a bit more how to work around these issues. This way we can do a 2.4.0 release with this fix, accompanied by release notes that show how to fix any problems people may experience. I'm sure there's some more code out there that relies on these false assumptions (most notably the fact that computeds generated by the mapping plugin have subtly different behavior from 'regular' computeds, as you have pointed out).

[sagacity]

There's an additional reason the timeout is there:

If you create a nested model, your parent model may also call ko.mapping.fromJS to create the children. If any of the children requires access to methods on the parent that only become available after the fromJS call then this will obviously break. But sometimes you cannot add these methods before you do the mapping, because they may also be dependent on the children being present.

This is a scenario that usually happens when you have limited control over the model that comes back from the server.

[drdamour]

i think the recommendation to fix the problems is anywhere a computed is backed by a standard variable, switch it to be backed by an observable.

for your other problem, can you put together a test that shows it not working? I can't quite see where you're going... I think there's a way to push DO's that need it onto a stack and unwind them at a later time but stil synchronously. Right now the code is evaluating them at the end of inner fromJS we could move that to the outer most fromJS somehow.

finally, it might make sense to enable the old behaviour via an option. Alternatively make it opt in to the new behaviour. for a 2.3.x release and deprecate the old behaviour.

[sagacity]

All good ideas!

For the synchronous unwinding I'll try to build a simple repro that illustrates my point. I saw in the current KO codebase there's some try/finally trickery going on, or are you referring to another way of doing this?

As for your last point, it depends on what we can do about the second point. If the behavior ends up being mostly identical for the vast majority of cases, we can make the new behavior the default. If it requires some rework on the user's part then it should probably be a global mapping setting that you need to opt-in to, and then in a next release deprecate the old behavior.

[drdamour]

i don't have the answer on how to do it, i don't yet understand the problem.

want to add the test to my fork?

[sagacity]

I did some more testing and it appears there had been some broken checkins that caused the bad behavior, and not your changes. So that's very good news, because it appears it does work as a drop-in replacement. However, there's still a window.setTimeout call remaining. In your pull request, what would you say of replacing the remaining window.setTimeout with a try/finally construction? So the mappingNesting = 0 statement should go in the finally part, and basically the entire implementation of fromJS can go in the try.

Also, can you maybe remove or disable any tests you consider to be erroneous?

Oh and feel free to let me know if anything is unclear or if you simply don't have the time to do all this, of course :)

[drdamour]

my commit https://github.com/drdamour/knockout.mapping/commit/6eab35f630f621b007d4445c51f4f3e7d903dfbe does remove and update some of the test cases already so you should be ok there.

The mapping nesting thing i guess is fine.

I'm worried that we aren't understanding why deferredObservables exist in the first place, is there a test case that is covering that?

[sagacity]

Sorry for not getting a test in order yet. Let's say you have a model with two child models a and b. If a has a computed that references a property in b, evaluating this computed will fail, since b and its properties don't exist yet. What's more, maybe b also has a dependency on a. Normally you would handle this with a deferEvaluation: true. However, the evaluation of the computed in a could have side effects (e.g. a method call) that some other code relies on. Maybe it initializes some state. This would break if you defer evaluation.

This is what the deferredObservable stuff tries to work around: It allows you to temporarily defer evaluation during creation of your (sub)model, but once that model is created it does evaluate your computed, making sure that any potential side-effects are also triggered. That's basically what the erroneous 'subscription' test is trying to check, but in doing so it relies on incorrect behavior.

[drdamour]

ok so something like this

var m = { 
   a : {  },
   b : 1
}

var m_mapping = {
  create : function(options)
  {
         var vm = {};
         vm.a = FromJS( options.data.a);
         vm.a.SiblingComputed = ko.computed(  function(){
                              vm.b()
                  });
         vm.b = FromJS( options.data.b );
       return vm;
  }
}

i guess? So just covering the case when the programmer put computed before mapping b? Is there a case where just re-ordering the statements in create wouldn't solve the problem?

[sagacity]

If a and b are created by the mapping plugin, through a nested create, you cannot determine the order. Also, sometimes the dependencies can be bidirectional.

[drdamour]

but even if a and b are created via a nested create...it's still the users code and they can put the computed wherever they want, right? i'm just not following the scenario you are trying to lie out. can you express it with code?

[sagacity]

Yeah, I'll need to do that to compensate for my poor explanation :)

[sagacity]

I put a sample here.

[drdamour]

ok i added 2 tests around this https://github.com/drdamour/knockout.mapping/commit/b662f0e49382b7653460d602639a39336b39fbf5 to the pull request. They passed without modification.

i also created a test runner that uses the release version of KO. This has failing tests, but they were failing before my changes too see https://github.com/SteveSanderson/knockout.mapping/issues/128

[sagacity]

Excellent, thanks! Now all that's needed is a simple workaround for the lack of hasWriteFunction in the release build of KO and I think we're good to release.

[sagacity]

It has now been merged, I also fixed the hasWriteFunction stuff, so it will just work with the release version of Knockout.

@drdamour @lisandropuzzolo : Can you guys confirm 0cfd93c3f55e07207ea7ec1b732f6b2562d047b0 works for you both (the js file in the root of the solution, not in the build folder)? If so, I'll bump up the version and do a release!

[drdamour]

some of my tests are failing...if i comment out the finally block they pass. I'll narrow down the underlying issue and get a unit test put together. I'm gonna guess nested is clearing out the stack of DO's to evaluate somehow.

[lisandropuzzolo]

This version is not passing my test neither... http://jsfiddle.net/lisandropuzzolo/adAmm/7/

With the a previous version, it worked: http://jsfiddle.net/lisandropuzzolo/sXZm6/1/

[drdamour]

stepping through my tests i see the finally block executing immediately after the return executes, setting the nesting to 0 prior to the countdown hitting 0.

what's the intent of setting the nesting to 0? to deal with the fact that a deferred computed eval may throw for the next from JS to succeed?

[drdamour]

There's a script error in proxydependentobservableBehaviors..i'm opening a pull request, this shows tests failing

[sagacity]

Yes, seems I was too hasty with the fix. Thanks for reviewing.

@drdamour It's indeed a way to reset the nesting count regardless of problems during mapping. This was previously done using the setTimeout call, but I'm hoping to remove all these async calls.

[sagacity]

@drdamour @lisandropuzzolo Can you guys test af662c614f14e793b33af64e934a412893bee1d1? It should be okay now. Instead of resetting this counter (which is basicaly I hack), the finally block now contains the decrease of the nesting count and all associated logic. I believe that's the correct place for it.