mbest / knockout-deferred-updates

Deferred Updates plugin for Knockout <=3.3.0
http://mbest.github.io/knockout-deferred-updates/
134 stars 23 forks source link

Experiencing lock-up (memory leak, +4MB/s, 30% CPU) when including this after knockout in chrome. #17

Closed gbmhunter closed 10 years ago

gbmhunter commented 10 years ago

Running chrome with this and the knockout library included in the HTML page, served with XAMPP for Windows.

gbmhunter commented 10 years ago

O.K., I've narrowed it down to a single instance of the series of JavaScript files that are called using this plugin. Will post more info as I narrow it down further.

gbmhunter commented 10 years ago

Narrowed down further, it has something to do with binding two comboboxes (<select> object in HTML) to the same ko.observable(). Only freezes when the knockout-deferred-updates library is included.

mbest commented 10 years ago

That's interesting. It could be a recursive update issue. Knockout prevents recursive updates to computed observables, and this plugin should also prevent most recursive updates, but it's possible that some can get through.

mbest commented 10 years ago

Okay, it's easy to duplicate:

<select data-bind="options: ['abc','def','ghi'], value: x"></select>
<select data-bind="options: ['xyz','uvw'], value: x"></select>

<script>
ko.applyBindings({ x: ko.observable() });
</script>
mbest commented 10 years ago

I haven't been able to think of a way to efficiently detect this type of recursion where computed A caused computed B to update, which updates A, and so on. Instead I've added a check that limits the maximum number of task "groups" and throws an exception if it goes over the limit (ad5a8bf). Thus it stops infinite recursion without blocking other types of complex scenarios.

In your particular case, the problem starts with the fact that the value binding tries to make the observable value equal to the currently selected item in the list. Because two different lists are bound to the same observable, this is really impossible, and the observable simply switches endlessly between the two values.

gbmhunter commented 10 years ago

Ah o.k. So knockout can easily detect this (and prevents it), but when using this plugin this becomes harder to detect?

Also note that when done with plain knockout, it kind of worked as expected, if I selected something from one list, the other list would update automatically (and the ko.observable() got updated).

How would you get two select lists to follow each other without hitting this recursion problem? Sorry if this seems obvious, I am pretty new to Javascript, I come from an embedded background

mbest commented 10 years ago

Knockout blocks recursive updates because it was easier to keep track of dependencies that way (knockout/knockout#387). This change was made in 2.1.0, and as you can see here (http://jsfiddle.net/9j2BQ/1/), with 2.0.0, we get an error from the browser (stack size). Even with 3.0.0 (http://jsfiddle.net/9j2BQ/), you can see in the console that the observable value switches between the two values whenever you change it.

I suggest you redesign your code to accurately represent the logic you desire. Otherwise, you're depending on the recursion being stopped at the right point (which could easily change in the future).

gbmhunter commented 10 years ago

I know this isn't probably the best place to ask, but incase there is a quick answer, how would I link two option (select) lists to the same value correctly i.e. if the user changes one the other one changes, and vice versa?

mbest commented 10 years ago

Given the example I provided where you want an observable to equal the last selected item from two select boxes with different available options, I'd use three observables, one each bound to the select boxes, with manual subscriptions from them to update the third.

var vm = {
    x1: ko.observable(),
    x2: ko.observable(),
    x: ko.observable()
};

function updateX(value) {
    vm.x(value);
}
vm.x1.subscribe(updateX);
vm.x2.subscribe(updateX);

http://jsfiddle.net/mbest/9j2BQ/2/

This might not be the best approach for you, though. I'd need to know more about your specific use-case to answer that.

gbmhunter commented 10 years ago

Hmmm, although it updated the underlying variable, it doesn't update the other select box when one is changed?

I am writing a online calculator framework called candy-calc (https://github.com/gbmhunter/candy-calc). Some of the calculators can be seen here (http://cladlab.com/electronics/general/online-calculators).

As you can see, each variable has selectable units. This is for linking the units of two calculator variables together, so if you change one, the other will change also (both in the backend variable and the front-end display).

By the way, I really appreciate your help here.

mbest commented 10 years ago

I see the units select boxes, but I don't see any where the units are linked together.

gbmhunter commented 10 years ago

That's what I'm trying to do. For the standard resistance calculator, I had the desired and actual resistance units linked together, so if you change the desired resistance units from Ohms to kOhms, the actual resistance units would change also. I did this by just pointing to the same ko.observable(). But when using the deferred updates plugin, this is what caused the crash, and so I'm looking for a proper way to re-implement this. Currently the two unit select boxes are independent.

mbest commented 10 years ago

Having both select boxes linked to the same value will work as long as the options are actually the same: http://jsfiddle.net/9j2BQ/3/

You have the options bound to objects, so they need to be the same objects.

gbmhunter commented 10 years ago

I still need two seperate unit variables though. They need to be linked in the code behind. This is what I have:

    this.linkUnits = function(calcVar1, calcVar2)
    {
        console.log('Linking units...');
        console.log('calcVar1.selUnit = ');
        console.log(calcVar1.selUnit());
        console.log('calcVar2.selUnit = ');
        console.log(calcVar2.selUnit());

        // Modified write function
        calcVar1.dispSelUnit = ko.computed({
            read : function(){
                console.log('Reading source var sel unit.');

                // Return as usual
                return calcVar1.selUnit();
            },
            write : function(value){
                // Write to both of them
                console.log('Writing ' + value + 'to both sel unit.');
                calcVar1.selUnit(value);
                calcVar2.selUnit(value);
            },
            owner : this
        });

        // Modified write function
        calcVar2.dispSelUnit = ko.computed({
            read : function(){
                console.log('Reading destination var sel unit.');

            // Make it equal to the source variables selected unit
                return calcVar2.selUnit();
            },
            write : function(value){
                // Write to both of them
                console.log('Writing ' + value + 'to both sel unit.');

                calcVar1.selUnit(value);
                calcVar2.selUnit(value);
            },
            owner : this
        });
    }

Where .selUnit() are just ko.observable().

But it just forever loops (i.e. recursive issues). I cannot see what is wrong with this!

mbest commented 10 years ago

You might be interested in this related question on Stack Overflow: http://stackoverflow.com/q/10809085/1287183

You're getting the recursion problem because the two variables have a different list of units. Although they display the same units, they are actually different objects. You need both variable to reference the same unit objects. In standard-resistance-finder.js, do this:

var resistenceUnits = [
    new cc.unit('m\u2126', 0.001),
    new cc.unit('\u2126', 1.0),
    new cc.unit('k\u2126', 1000.0)
];

this.desiredRes = new cc.input(
    this,
    function() { return true; },
    resistenceUnits,
    0);

...

this.actualRes = new cc.output(
    this,
    ...
    resistenceUnits,
    0, 2);
gbmhunter commented 10 years ago

I have posted this issue on stack overflow here http://stackoverflow.com/questions/20436024/connecting-two-knockout-variables-together-without-causing-recursion

gbmhunter commented 10 years ago

You fixed it! Genius! I'll accept your answer on Stack Overflow. Many thanks!!! Interesting bug...!