subscribing to a computed when mapping about knockout.mapping HOT 21 CLOSED

Fix up the first example by saving marketChange in self (this) to avoid influence of GC.

from knockout.mapping.

no effect

http://jsfiddle.net/x3PLB/1/

from knockout.mapping.

http://jsfiddle.net/x3PLB/2/

During mapping, evaluation of computeds is deferred so that you can nest them properly. Their first evaluation then happens once all processing is complete (using window.setTimeout). Then, there is some code that checks if you have already evaluated it yourself. In this example case, explcitly evaluating the computed before subscribing solves the issue.

I have to admit I think it should work even if you didn't explicitly evaluate the computed. Somehow KO's dependency detection fails when you don't. If you could take a bit of time to figure out why this is happening it would be highly appreciated, otherwise the aforementioned fix should help.

from knockout.mapping.

Thanks Roy. That is a better workaround. I'll use that.

I'll have a look to see if i can follow the dependancy detection through.

I've found that this is unrelated to throttling which might simplify things a bit.

http://jsfiddle.net/x3PLB/3/

from knockout.mapping.

Ok. I think I found it !!

In withProxyDependentObservable you override ko.dependantObservable to return a wrapped DO.

You maintain a list of unwrapped DOs in the array dependantObservables (line 244-245)

In exports.fromJS you loop over dependantObservables and evaluate them. (line 101)

The wrapper is never evaluated, and since I'm subscribing to the wrapper my function is never called.

Not sure of the best solution but if you stored the wrapped DO in the array it should all work.

So just swap lines 244 and 245:

  realDependentObservable = wrap(realDependentObservable);
  dependentObservables.push(realDependentObservable);

from knockout.mapping.

Of course evaluating the wrapper will remove it from dependentObservables potentially breaking the loop. you could of course use while(length>0){pop} to avoid the problem.

from knockout.mapping.

Yes, spot on. Thanks for taking the time to dig a bit deeper. I'm preparing a fix right now, based on these suggestions. I'm also adding a unit test based on your original fiddle to make sure the problem stays away.

from knockout.mapping.

Great work!

I wanted to see whether you needed a doubly nested model (to simplify the test). But the fiddle just worked! LOL. It was because i'm using the code in the master branch which has been fixed. :)

This is a version before the fix with only a single nested object and you do still get the bug.
http://jsfiddle.net/x3PLB/4/

That being said. Normally, when i subscribe to a DO, the function only gets called after the first change. in this case it gets called by fromJS when it's finished mapping my objects. Is the only way to get this behaviour to manually evaluate the DO before subscribing?

I guess I could set deferEvaluation: true on my DO to avoid the wrapping but I guess I'd still need to manually evaluate it before subscribing?

Thanks again!

from knockout.mapping.

Currently all DO's are wrapped, whether deferEvaluation is set or not. So they always get evaluated eventually. If that's a problem you could certainly add a check if deferEvaluation is set in the wrapper function and decide whether to call DO.apply.

Maybe you can play around a bit and report your findings? It's definitely an easy fix to do.

from knockout.mapping.

No. It isn't wrapped if your DO already has deferEvaluation set.

if (!realDeferEvaluation) {
  dependentObservables.push(realDependentObservable);
  realDependentObservable = wrap(realDependentObservable);
}

of course for this to work correctly you'd need to swap

var realDeferEvaluation = options.deferEvaluation;

and

if (read && typeof read == "object") { // mirrors condition in knockout implementation of DO's
  options = read;
}

Currently if you did ko.computed({read: function(){...}, deferEvalutaion: true}) it would be ignored. You would need to do ko.computed(function(){...}, null, {deferEvaluation: true})

from knockout.mapping.

Oh, right. Apologies for the misinformation, I'm juggling too many things at the moment. :) If you feel so inclined, a unit test + pull request for this functionality is more than welcome!

from knockout.mapping.

In trying to test the behaviour described above. I have uncovered a more serious bug. I will start a new Issue for it.

from knockout.mapping.

Ok. I've fixed issue #71 but now i don't think there's any way of testing the above behaviour without putting in some debug code like if(DEBUG) wrapper._wrapper = true;.

I've tried

    var realDO = ko.dependentObservable;
    ko.dependentObservable = function(read, owner, options){
        var result = realDO(read, owner, options);
        result.real = ko.dependentObservable.real;
        return result;
    }
    ko.dependentObservable.real = true;

    var mapped = ko.mapping.fromJS(obj, mapping);

    equal(mapped.inner.DO1.real, true)
    equal(mapped.inner.DO2.real, undefined)

but of course realKoDependentObservable is set before my test runs and there's no way to override it.

Any thoughts?

from knockout.mapping.

I think the DEBUG check would actually be fine. It will get compiled out by the Closure compiler. As long as your debug code doesn't have any side-effects I think it is actually quite valid to expose some additional data to aid unit testing.

from knockout.mapping.

Just realised knockout.mapping doesn't support the DEBUG check. In knockout it is added in the build script

https://github.com/SteveSanderson/knockout/blob/master/build/build-windows.bat#L31

But this would require restructuring the code a bit.

Would you be cool to add this?

from knockout.mapping.

Are you asking if it's okay for you to add it, or if I want to add it? :) In both cases the answer is 'yes' but I am not currently behind a coding machine.

from knockout.mapping.

I was asking if you'd like to. If you let me know what it's in I'll add this test+fix.

from knockout.mapping.

Ok! I'll try to take some time over the weekend to do this.

from knockout.mapping.

James, the DEBUG conditional is added in cde87aa.

from knockout.mapping.

Thanks Roy.

Pull request 73 includes pull request 72 which I rebased to the current master. Don't use github to merge both because i think you'll end up with duplicate commits. It's a shame github doesn't allow a fasforward merge :( Probably best to merge outside of github and push.

Thanks

James

from knockout.mapping.

Merge complete! I'll create a v2.1.2 release from this.

from knockout.mapping.