Skip to content

deferred updates: only notify computed changed if it was evaluated#2240

Merged
mbest merged 1 commit intomasterfrom
2240-deferred-computed-notify-update
Jun 3, 2017
Merged

deferred updates: only notify computed changed if it was evaluated#2240
mbest merged 1 commit intomasterfrom
2240-deferred-computed-notify-update

Conversation

@mbest
Copy link
Copy Markdown
Member

@mbest mbest commented May 29, 2017

Currently, if a computed has a non-primitive value, it will always notify a change if it was marked as dirty. It should only notify a change if it was actually recalculated. Thus it's not sufficient to base the notify decision solely on the value, as is currently done.

@fastfasterfastest
Copy link
Copy Markdown

Is it possible for you to provide a sample using the latest knockout (3.4.2) showing the incorrect behavior?

It should only notify a change if it was actually recalculated.

Is it possible for a computed to be marked dirty and then not be recalculated, i.e. for it to be marked dirty and then somehow "get discarded"?
Or do you mean it should only notify a change if its new value, when recalculated, is different (using its equalityComparer) than its old value?

@mbest
Copy link
Copy Markdown
Member Author

mbest commented May 12, 2017

https://jsfiddle.net/mbest/zvqv620e/

function ViewModel() {
    var self = this;
    var someObject = { x: 1 };
    
    self.obs = ko.observable('xxx');
    self.isTruthy = ko.pureComputed(function() {return !!self.obs();});
    self.objIfTruthy = ko.pureComputed(function() { return self.isTruthy() ? someObject : null;});
    
    self.log = ko.observableArray();
    self.objIfTruthy.subscribe(function(value) {
    	self.log.push(ko.toJSON(value));
    });
};

ko.options.deferUpdates = true;
ko.applyBindings(new ViewModel());

@fastfasterfastest
Copy link
Copy Markdown

Thanks.

The behavior of the default equalityComparer often surprises me, and yet again, and you have already helped me with that before, e.g. #1884

In this case, because object equality is not good enough for the default equalityComparer the behavior is perhaps surprising. But, isn't this by design? E.g., see http://stackoverflow.com/questions/12714761/why-does-knockout-jss-default-equality-comparer-treat-non-primitive-types-as-no

You of course already know, but with an "appropriate" equalityComparer on the computed the behavior is as expected(?), https://jsfiddle.net/fastfasterfastest/2e7v798x/

Is this an issue with deferUpdates though? In your sample, the isTruthy computed masks the same/similar behavior when deferUpdates=false, because isTruthy has a primitive value and thus the computed in question never gets notified.
Changing your sample, getting rid of the isTruthy computed and setting deferUpdates=false you observe same behavior as your original sample does. So I think the title of this issue might be misleading - the behavior reported can be seen and experienced regardless of the value of deferUpdates.

https://jsfiddle.net/fastfasterfastest/n1vbj5dc/

function ViewModel() {
    var self = this;
    var someObject = { x: 1 };

    self.obs = ko.observable('xxx');
    self.objIfTruthy = ko.pureComputed(function() { return !!self.obs() ? someObject : null;});
    
    self.log = ko.observableArray();
    self.objIfTruthy.subscribe(function(value) {
        self.log.push(ko.toJSON(value));
    });
};

ko.options.deferUpdates = false;
ko.applyBindings(new ViewModel());

@fastfasterfastest
Copy link
Copy Markdown

@mbest Do you agree that this issue has nothing to do with deferUpdates but instead is due to the behavior of the default equalityComparer? If so, is the plan to change the behavior of the default equalityComparer?

@mbest
Copy link
Copy Markdown
Member Author

mbest commented May 23, 2017

The default equalityComparer is fine. The problem is that when using deferUpdates, only the equalityComparer is used to determine if there was a change, but it would be good to also check if the computed was actually re-evaluated.

@fastfasterfastest
Copy link
Copy Markdown

I didn't say that there was a problem with default equalityComparer - I merely stated that I think the behavior observed is due to the behavior of the default equalityComparer.

The problem is that when using deferUpdates, only the equalityComparer is used to determined if there was a change

When not using deferUpdates, only the equalityComparer is used to determine if there was a change as well. The sample I provide above and in https://jsfiddle.net/fastfasterfastest/n1vbj5dc/, shows that the behavior is the same regardless of the value of deferUpdates. Or am I missing something?

Currently, if a computed has a non-primitive value, it will always notify a change if it was marked as dirty. It should only notify a change if it was actually recalculated. Thus it's not sufficient to base the notify decision solely on the value, as is currently done.

Today, with deferUpdates=false, if a computed has a non-primitive value, it will always notify a change when it is re-evaluated.
Today, with deferUpdates=true, if a computed has a non-primitive value, it will always notify a change when it is re-evaluated.

I am not sure I am convinced there is an issue here. I am concerned about #2234 being fixed, though.

@mbest
Copy link
Copy Markdown
Member Author

mbest commented May 23, 2017

Today, with deferUpdates=true, if a computed has a non-primitive value, it will always notify a change when it is re-evaluated.

The problem is that with deferUpdates=true, a computed with a non-primitive value will notify a change even if it wasn't evaluated but just was marked as dirty.

@fastfasterfastest
Copy link
Copy Markdown

with deferUpdates=true, a computed with a non-primitive value will notify a change even if it wasn't evaluated

How is that possible - how can a computed observable notify subscribers of a change in its value without (first) being re-evaluated to determine the value with which to notify?

@fastfasterfastest
Copy link
Copy Markdown

Is it possible for a computed to be marked dirty and then not be recalculated

I traced into the sources, and it seems the answer to my question is yes. At least when deferUpdates=true, a computed seemingly can become dirty yet not be re-evaluated because it is/was not "stale".

I had thought that if a computed was marked dirty it would (eventually) be re-evaluated, thus my comments above.

@mbest
Copy link
Copy Markdown
Member Author

mbest commented May 29, 2017

I've attached the fix here.

@mbest mbest force-pushed the 2240-deferred-computed-notify-update branch from 847d1f1 to fa17c59 Compare June 2, 2017 06:09
@mbest mbest merged commit d20958b into master Jun 3, 2017
@mbest mbest deleted the 2240-deferred-computed-notify-update branch July 19, 2017 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants