subscribing to a computed when mapping

[jamesfoster]

Hi I've set up a simple demo of the issue I'm having

http://jsfiddle.net/x3PLB/

I'm not sure if it requires a doubly nested model but it more accurately reflects my scenario. If you look at InnerInner you can see I've setup a computed which merely depends on X Y and Z. and a subscription to that computed which prints "test" to the console.

The idea is to throttle these three observables together.

I seems that because ko.computed is being overridden while mapping, throttling doesn't work. The throttle extender returns a wrapped computed and so does knockout.mapping... maybe there's a clash there?

One work around I've found is...

    var marketChange = ko.computed(function () {
        self.X();
        self.Y();
        self.Z();
    });

    marketChange.extend({ throttle: 500 });

    setTimeout(function () {
        marketChange.subscribe(function () {
            console.log('test');
        });
    }, 100);

However, this wouldn't work for a writable computed (which is why it requires a wrapper).

(The delay is required to avoid raising the subscribe method before the first change.)

Any help would be appreciated.

Cheers

Update: I've found that this is not related to throttling
http://jsfiddle.net/x3PLB/3/

[DzenisevichK]

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

[jamesfoster]

no effect

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

[sagacity]

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.

[jamesfoster]

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/

[jamesfoster]

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

[jamesfoster]

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.

[sagacity]

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.

[jamesfoster]

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!

[sagacity]

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.

[jamesfoster]

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

[sagacity]

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!

[jamesfoster]

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

[jamesfoster]

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?

[sagacity]

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.

[jamesfoster]

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?

[sagacity]

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.

[jamesfoster]

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

[sagacity]

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

[sagacity]

James, the DEBUG conditional is added in cde87aa.

[jamesfoster]

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

[sagacity]

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