Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Strange behaviour on consecutive .fromJS() calls #95

Open
lisandropuzzolo opened this Issue · 39 comments

3 participants

@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.

@RoyJacobs
Collaborator

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.

@RoyJacobs
Collaborator

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 9a38c5d

@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

@RoyJacobs
Collaborator

@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!

@RoyJacobs
Collaborator

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).

@RoyJacobs
Collaborator

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.

@RoyJacobs
Collaborator

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?

@RoyJacobs
Collaborator

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 drdamour/knockout.mapping@6eab35f 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?

@RoyJacobs
Collaborator

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?

@RoyJacobs
Collaborator

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?

@RoyJacobs
Collaborator

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

@RoyJacobs
Collaborator

I put a sample here.

@drdamour

ok i added 2 tests around this drdamour/knockout.mapping@b662f0e 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 #128

@RoyJacobs
Collaborator

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.

@RoyJacobs
Collaborator

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 0cfd93c 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

@RoyJacobs
Collaborator

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.

@RoyJacobs
Collaborator

@drdamour @lisandropuzzolo Can you guys test af662c6? 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.

@drdamour

i don't think this is what you want either. consider this scenario: something goes wrong during mapping. Do you really want to attempt to evaluate computed variables? won't they very likely throw an exception themselves since something went wrong previously?

@RoyJacobs
Collaborator

That's a fair point, although you are likely to be in an error situation anyway, at that point. If this is a situation you expect there would already be exception handling on the caller's side, I would imagine.

Additionally, when an exception occurs during the mapping process, I would assume you don't want to continue working with that model anyway (regardless if computeds are initialized or not).

@drdamour

so the wrong exception will be thrown in the finally block hiding the real exceptions (not that JS exceptions are that meaningful, but..)
I think you want to put the setting to 0 in the catch statement. if anything goes wrong you reset it to 0. if everything goes right then the normal operation will decrement it down to 0 eventually.

rethrow in the catch (can js do that?)

@RoyJacobs
Collaborator

So something like replacing the finally with this? It will properly propagate exceptions but also reset the faulty state.

        } catch(e) {
            mappingNesting = 0;
            throw e;
        }
@RoyJacobs
Collaborator

This is implemented in f85cf7d.

@drdamour drdamour referenced this issue from a commit
Chris DaMour Created a test for #95 e67e2e0
@drdamour

this new implementation makes my tests pass. I added a test for this scenario with my pull.

@RoyJacobs
Collaborator

@lisandropuzzolo Can you also confirm the latest build works for you?
@drdamour Thanks for all the hard work!

@lisandropuzzolo

¡Yes!
¡My test passes now!
http://jsfiddle.net/lisandropuzzolo/5suAD/1/

Thank you very much!
I'll be doing more test and post here if I found some problem.

@RoyJacobs
Collaborator

Ok good news! I still have some failing tests in our codebase but I think they're more related to some kind of race condition that accidentally made the tests pass in the "old" version of the plugin.

If all is well, I'll do a release after the weekend.

@supergibbs supergibbs referenced this issue from a commit in supergibbs/knockout.mapping
Chris DaMour removed the setTimeoutas it doesn't seem to be logical to do things a…
…fter fromJS returns and it breaks KO validation plugin's behaviour. Tweaked some unit tests per evaluation at SteveSanderson#95 (comment)
6eab35f
@supergibbs supergibbs referenced this issue from a commit in supergibbs/knockout.mapping
Chris DaMour added tests regarding concerns of circular and nested computed observ…
…ables as discussed at SteveSanderson#95
b662f0e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.