Skip to content

Commit

Permalink
Enhance fix for #1975 to only re-evaluate if the awakened dependency …
Browse files Browse the repository at this point in the history
…changes. Also add a lower-level test.
  • Loading branch information
mbest committed Jan 15, 2016
1 parent eb88065 commit 241c26c
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 5 deletions.
23 changes: 23 additions & 0 deletions spec/pureComputedBehaviors.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,29 @@ describe('Pure Computed', function() {
expect(computed.getDependenciesCount()).toEqual(0);
});

it('Should reevaluate if dependency was changed during awakening, but not otherwise', function() {
// See https://github.com/knockout/knockout/issues/1975
var data = ko.observable(0),
isEven = ko.pureComputed(function() { return !(data() % 2); }),
timesEvaluated = 0,
pureComputed = ko.pureComputed(function () { ++timesEvaluated; return isEven(); }),
subscription;

expect(pureComputed()).toEqual(true);
expect(timesEvaluated).toEqual(1);

data(1);
subscription = isEven.subscribe(function() {});
expect(pureComputed()).toEqual(false);
expect(timesEvaluated).toEqual(2);
subscription.dispose();

data(3);
subscription = isEven.subscribe(function() {});
expect(pureComputed()).toEqual(false);
expect(timesEvaluated).toEqual(2);
});

describe('Should maintain order of subscriptions', function () {
var data, dataPureComputed;

Expand Down
19 changes: 14 additions & 5 deletions src/subscribables/dependentObservable.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ var computedFn = {
evaluateImmediate: function (notifyChange) {
var computedObservable = this,
state = computedObservable[computedState],
disposeWhen = state.disposeWhen;
disposeWhen = state.disposeWhen,
changed = false;

if (state.isBeingEvaluated) {
// If the evaluation of a ko.computed causes side effects, it's possible that it will trigger its own re-evaluation.
Expand Down Expand Up @@ -238,22 +239,25 @@ var computedFn = {

state.isBeingEvaluated = true;
try {
this.evaluateImmediate_CallReadWithDependencyDetection(notifyChange);
changed = this.evaluateImmediate_CallReadWithDependencyDetection(notifyChange);
} finally {
state.isBeingEvaluated = false;
}

if (!state.dependenciesCount) {
computedObservable.dispose();
}

return changed;
},
evaluateImmediate_CallReadWithDependencyDetection: function (notifyChange) {
// This function is really just part of the evaluateImmediate logic. You would never call it from anywhere else.
// Factoring it out into a separate function means it can be independent of the try/catch block in evaluateImmediate,
// which contributes to saving about 40% off the CPU overhead of computed evaluation (on V8 at least).

var computedObservable = this,
state = computedObservable[computedState];
state = computedObservable[computedState],
changed = false;

// Initially, we assume that none of the subscriptions are still being used (i.e., all are candidates for disposal).
// Then, during evaluation, we cross off any that are in fact still being used.
Expand Down Expand Up @@ -289,11 +293,15 @@ var computedFn = {
} else if (notifyChange) {
computedObservable["notifySubscribers"](state.latestValue);
}

changed = true;
}

if (isInitial) {
computedObservable["notifySubscribers"](state.latestValue, "awake");
}

return changed;
},
evaluateImmediate_CallReadThenEndDependencyDetection: function (state, dependencyDetectionContext) {
// This function is really part of the evaluateImmediate_CallReadWithDependencyDetection logic.
Expand Down Expand Up @@ -367,8 +375,9 @@ var pureComputedOverrides = {
state.dependencyTracking = null;
state.dependenciesCount = 0;
state.isStale = true;
computedObservable.evaluateImmediate();
computedObservable.updateVersion();
if (computedObservable.evaluateImmediate()) {
computedObservable.updateVersion();
}
} else {
// First put the dependencies in order
var dependeciesOrder = [];
Expand Down

0 comments on commit 241c26c

Please sign in to comment.