From ae1718881572e230432b15c16f7f24654485e16c Mon Sep 17 00:00:00 2001 From: Rafael Weinstein Date: Thu, 19 Dec 2013 17:03:04 -0800 Subject: [PATCH] This patch makes a set of related changes: 1) Ensure that deliver fully flushes dependencies. This is required so that a computedProperty can ensure that it synchronously returns the correct value. The main change is that PathObservers (normal and Compound) must dirtyCheck even in Object.observe when deliver() is called. Dirty-checking touches all dependent values -- some of which may be computed properties, which, in turn flush their dependencies. 2 ) Remove getValue from Observable interface. Now that deliver() ensure transitive flushing, synchronous getValue is no longer need 3) Observer.defineProperty -> Observer.defineComputedProperty for clarity 4) ArrayReduction now implements the Observable interface and removes ArrayReduction.defineProperty, in favor of Observer.defineComputedProperty R=arv BUG= Review URL: https://codereview.appspot.com/44050049 --- src/observe.js | 157 ++++++++++++++-------------------- tests/test.js | 54 +++++++++--- tests/test_array_reduction.js | 18 ++-- util/array_reduction.js | 105 +++++++++++------------ 4 files changed, 160 insertions(+), 174 deletions(-) diff --git a/src/observe.js b/src/observe.js index c70f464..2a2c973 100644 --- a/src/observe.js +++ b/src/observe.js @@ -283,6 +283,8 @@ } if (global.testingExposeCycleCount) global.dirtyCheckCycleCount = cycles; + + return cycles > 0; } function objectIsEmpty(object) { @@ -379,10 +381,6 @@ return this.value_; }, - getValue: function() { - return this.value_; - }, - internalCallback_: function(records) { if (this.state_ != OPENED) return; @@ -408,16 +406,23 @@ if (this.state_ != OPENED) return; - if (hasObserve) { - if (this.deliverDeps_) - this.deliverDeps_(); - - this.testingResults = testingResults; - Object.deliverChangeRecords(this.boundInternalCallback_); - this.testingResults = undefined; - } else { + if (!hasObserve) { dirtyCheck(this); + return; } + + this.testingResults = testingResults; + if (this.deliverDirtyChecks_) { + var anyChanged = dirtyCheck(this); + if (testingResults) + testingResults.anyChanged = testingResults.anyChanged || anyChanged; + + this.reporting_ = false; + } + + Object.deliverChangeRecords(this.boundInternalCallback_); + this.testingResults = undefined; + this.reporting_ = true; }, report_: function() { @@ -690,18 +695,16 @@ function PathObserver(object, path) { Observer.call(this); + this.object_ = object; this.path_ = path instanceof Path ? path : getPath(path); this.observedSet_ = undefined; } PathObserver.prototype = createObject({ + deliverDirtyChecks_: true, __proto__: Observer.prototype, - getValue: function() { - return this.path_.getValueFrom(this.object_); - }, - connect_: function() { if (hasObserve) this.observedSet_ = new ObservedSet(this.boundInternalCallback_); @@ -737,15 +740,16 @@ }, sync_: function(hard) { - if (hard) { - if (this.observedSet_) - this.observedSet_.reset(); + if (!hard) + return; - this.value_ = this.path_.getValueFrom(this.object_, this.observedSet_); + if (this.observedSet_) + this.observedSet_.reset(); - if (this.observedSet_) - this.observedSet_.cleanup(); - } + this.value_ = this.path_.getValueFrom(this.object_, this.observedSet_); + + if (this.observedSet_) + this.observedSet_.cleanup(); }, setValue: function(newValue) { @@ -757,29 +761,39 @@ function CompoundObserver() { Observer.call(this); - this.observed_ = []; this.value_ = []; - this.hasObservers_ = false; - this.depsChanged_ = undefined; + this.observedSet_ = undefined; + this.observed_ = []; } var observerSentinel = {}; CompoundObserver.prototype = createObject({ - __proto__: PathObserver.prototype, + deliverDirtyChecks_: true, + __proto__: Observer.prototype, - getValue: function() { - var values = []; - this.getValues_(values, true); - return values; + connect_: function() { + if (hasObserve) + this.observedSet_ = new ObservedSet(this.boundInternalCallback_); }, - setValue: function() { - console.warn('Set to CompoundObserver ignored.'); + disconnect_: function() { + this.value = undefined; + this.value_ = undefined; + + if (this.observedSet_) { + this.observedSet_.reset(); + this.observedSet_.cleanup(); + this.observedSet_ = undefined; + } + + for (var i = 0; i < this.observed_.length; i += 2) { + if (this.observed_[i] === observerSentinel) + this.observed_[i + 1].close(); + } + this.observed_ = undefined; }, - // TODO(rafaelw): Consider special-casing when |object| is a PathObserver - // and path 'value' to avoid explicit observation. addPath: function(object, path) { if (this.state_ != UNOPENED) throw Error('Cannot add paths once started.'); @@ -791,35 +805,12 @@ if (this.state_ != UNOPENED) throw Error('Cannot add observers once started.'); - var value = observer.open(this.observerChanged_, this); - this.hasObservers_ = true; + var value = observer.open(this.deliver, this); this.observed_.push(observerSentinel, observer); this.value_.push(value); }, - deliverDeps_: function() { - if (!this.hasObservers_) - return; - - for (var i = 0; i < this.observed_.length; i += 2) { - if (this.observed_[i] === observerSentinel) - this.observed_[i + 1].deliver(); - } - }, - - observerChanged_: function() { - if (!hasObserve) - return this.deliver(); - - if (!this.depsChanged_) { - this.depsChanged_ = {}; - Object.observe(this.depsChanged_, this.boundInternalCallback_); - } - - this.depsChanged_.changed = !this.depsChanged_.changed; - }, - - getValues_: function(values, sync) { + getValues_: function(sync) { if (this.observedSet_) this.observedSet_.reset(); @@ -832,16 +823,16 @@ pathOrObserver.getValueFrom(object, this.observedSet_) if (sync) { - values[i / 2] = value; + this.value_[i / 2] = value; continue; } - if (areSameValue(value, values[i / 2])) + if (areSameValue(value, this.value_[i / 2])) continue; oldValues = oldValues || []; - oldValues[i / 2] = values[i / 2]; - values[i / 2] = value; + oldValues[i / 2] = this.value_[i / 2]; + this.value_[i / 2] = value; } if (this.observedSet_) @@ -851,7 +842,7 @@ }, check_: function() { - var oldValues = this.getValues_(this.value_); + var oldValues = this.getValues_(); if (!oldValues) return; @@ -863,26 +854,7 @@ sync_: function(hard) { if (hard) - this.getValues_(this.value_, true); - }, - - close: function() { - if (this.hasObservers_) { - for (var i = 0; i < this.observed_.length; i += 2) { - if (this.observed_[i] === observerSentinel) - this.observed_[i + 1].close(); - } - - this.observed_ = undefined; - this.value_ = undefined; - } - - if (this.depsChanged_) { - Object.unobserve(this.depsChanged_, this.boundInternalCallback_); - this.depsChanged_ = undefined; - } - - Observer.prototype.close.call(this); + this.getValues_(true); } }); @@ -919,10 +891,6 @@ this.callback_.call(this.target_, this.value_, oldValue); }, - getValue: function() { - return this.getValueFn_(this.observable_.getValue()); - }, - discardChanges: function() { this.value_ = this.getValueFn_(this.observable_.discardChanges()); return this.value_; @@ -972,16 +940,18 @@ } } - Observer.defineProperty = function(target, name, observable) { + Observer.defineComputedProperty = function(target, name, observable) { var notify = notifyFunction(target, name); - observable.open(function(newValue, oldValue) { + var value = observable.open(function(newValue, oldValue) { + value = newValue; if (notify) notify(PROP_UPDATE_TYPE, oldValue); }); Object.defineProperty(target, name, { get: function() { - return observable.getValue(); + observable.deliver(); + return value; }, set: function(newValue) { observable.setValue(newValue); @@ -992,10 +962,9 @@ return { close: function() { - var lastValue = observable.getValue(); observable.close(); Object.defineProperty(target, name, { - value: lastValue, + value: value, writable: true, configurable: true }); diff --git a/tests/test.js b/tests/test.js index 94e765c..da99d27 100644 --- a/tests/test.js +++ b/tests/test.js @@ -333,11 +333,9 @@ suite('ObserverTransform', function() { observer.setValue(2); assert.strictEqual(obj.foo, 1); - assert.strictEqual(2, observer.getValue()); assertPathChanges(2, 4); obj.foo = 10; - assert.strictEqual(20, observer.getValue()); assertPathChanges(20, 2); observer.close(); @@ -804,10 +802,10 @@ suite('PathObserver Tests', function() { observer = new PathObserver(model, 'a.b'); observer.open(callback); _b = 3; // won't be observed. - assertNoChanges(); + assertPathChanges(3, 2); model.a.b = 4; // will be observed. - assertPathChanges(4, 2); + assertPathChanges(4, 3); observer.close(); }); @@ -825,13 +823,13 @@ suite('PathObserver Tests', function() { var b = {}; var c = {}; - root.a.observer = Observer.defineProperty(root.a, 'value', + root.a.observer = Observer.defineComputedProperty(root.a, 'value', new PathObserver(root, 'value')); - root.a.b.observer = Observer.defineProperty(root.a.b, 'value', + root.a.b.observer = Observer.defineComputedProperty(root.a.b, 'value', new PathObserver(root.a, 'value')); - root.c.observer = Observer.defineProperty(root.c, 'value', + root.c.observer = Observer.defineComputedProperty(root.c, 'value', new PathObserver(root, 'value')); root.c.value = 2; @@ -856,7 +854,7 @@ suite('PathObserver Tests', function() { Object.observe(target, callback); } - var observer = Observer.defineProperty(target, 'computed', + var observer = Observer.defineComputedProperty(target, 'computed', new PathObserver(source, 'foo.bar')); assert.isTrue(target.hasOwnProperty('computed')); @@ -897,6 +895,36 @@ suite('PathObserver Tests', function() { name: 'computed', type: Observer.changeRecordTypes.add }, + { + object: target, + name: 'computed', + type: Observer.changeRecordTypes.update, + oldValue: 1 + }, + { + object: target, + name: 'computed', + type: Observer.changeRecordTypes.update, + oldValue: 3 + }, + { + object: target, + name: 'computed', + type: Observer.changeRecordTypes.update, + oldValue: 5 + }, + { + object: target, + name: 'computed', + type: Observer.changeRecordTypes.update, + oldValue: 7 + }, + { + object: target, + name: 'computed', + type: Observer.changeRecordTypes.update, + oldValue: undefined + }, { object: target, name: 'computed', @@ -909,14 +937,14 @@ suite('PathObserver Tests', function() { test('DefineProperty - empty path', function() { var target = {} - var observer = Observer.defineProperty(target, 'foo', - new PathObserver(1)); + var observer = Observer.defineComputedProperty(target, 'foo', + new PathObserver(1)); assert.isTrue(target.hasOwnProperty('foo')); assert.strictEqual(1, target.foo); var obj = {}; - var observer2 = Observer.defineProperty(target, 'bar', - new PathObserver(obj)); + var observer2 = Observer.defineComputedProperty(target, 'bar', + new PathObserver(obj)); assert.isTrue(target.hasOwnProperty('bar')); assert.strictEqual(obj, target.bar); }); @@ -956,7 +984,6 @@ suite('CompoundObserver Tests', function() { model.b = 3; model.c = 4; - assert.deepEqual([2, 3, 4], observer.getValue()); assertCompoundPathChanges([2, 3, 4], ['a', 20, 'c'], observerCallbackArg); @@ -966,7 +993,6 @@ suite('CompoundObserver Tests', function() { assert.deepEqual(['z', 'y', 'x'], observer.discardChanges()); assertNoChanges(); - observer.setValue('blarg!'); assert.strictEqual('z', model.a); assert.strictEqual('y', model.b); assert.strictEqual('x', model.c); diff --git a/tests/test_array_reduction.js b/tests/test_array_reduction.js index 4c3658e..4a4b6c8 100644 --- a/tests/test_array_reduction.js +++ b/tests/test_array_reduction.js @@ -10,15 +10,13 @@ suite('ArrayReduction Tests',function(){ {id:1} ]; - reductionObserver = ArrayReduction.defineProperty(obj,'reducedValue',{ - array: array, - path: 'id', - initial: [], - reduce: function(value,cur,i){ - value.push([cur.id,i]); - return value; - } - }); + var reduction = new ArrayReduction(array, 'id', function(value, cur, i) { + value.push([cur.id,i]); + return value; + }, []); + + reductionObserver = Observer.defineComputedProperty(obj,'reducedValue', + reduction); }); teardown(function(){ @@ -77,4 +75,4 @@ suite('ArrayReduction Tests',function(){ [ 1,2] ]); }); -}); \ No newline at end of file +}); diff --git a/util/array_reduction.js b/util/array_reduction.js index 8c3958f..3a4c06a 100644 --- a/util/array_reduction.js +++ b/util/array_reduction.js @@ -13,28 +13,45 @@ // limitations under the License. function ArrayReduction(array, path, reduceFn, initial) { - this.array = array; - this.path = path ? path.trim() : undefined; + this.callback_ = undefined; + this.target_ = undefined; + this.value_ = undefined; + this.array_ = array; + this.path_ = Path.get(path); this.reduceFn = reduceFn; this.initial = initial; - this.arrayObserver = new ArrayObserver(array); - this.arrayObserver.open(this.handleSplices, this); - this.observers = this.path ? [] : undefined; - - this.handleSplices([{ - index: 0, - removed: [], - addedCount: array.length - }]); } ArrayReduction.prototype = { - updateObservers: function(splices) { + open: function(callback, target) { + this.callback_ = callback; + this.target_ = target; + + this.arrayObserver = new ArrayObserver(this.array_); + this.arrayObserver.open(this.handleSplices, this); + this.observers = this.path_.valid ? [] : undefined; + + this.handleSplices([{ + index: 0, + removed: [], + addedCount: this.array_.length + }]); + + return this.value_; + }, + + handleSplices: function(splices) { + this.reduce(); + + if (!this.observers) + return; + for (var i = 0; i < splices.length; i++) { var splice = splices[i]; var added = []; for (var j = 0; j < splice.addedCount; j++) { - var observer = new PathObserver(this.array[splice.index + j], this.path); + var observer = new PathObserver(this.array_[splice.index + j], + this.path_); observer.open(this.reduce, this); added.push(observer); @@ -49,24 +66,36 @@ ArrayReduction.prototype = { } }, - handleSplices: function(splices) { - if (this.observers) - this.updateObservers(splices); + reduce: function(sync) { + var value = this.array_.reduce(this.reduceFn, this.initial); + if (value == this.value_) + return; - this.reduce(); - }, + var oldValue = this.value_; + this.value_ = value; - reduce: function() { - this.value = this.array.reduce(this.reduceFn, this.initial); + if (!sync) + this.callback_.call(this.target_, this.value_, oldValue); }, close: function() { this.observers.forEach(function(observer) { observer.close(); }); + this.arrayObserver.close(); }, + discardChanges: function() { + this.arrayObserver.discardChanges(); + this.observers.forEach(function(observer) { + observer.discardChanges(); + }); + + this.reduce(true); + return this.value_; + }, + deliver: function() { this.arrayObserver.deliver(); this.observers.forEach(function(observer) { @@ -74,39 +103,3 @@ ArrayReduction.prototype = { }); } } - -ArrayReduction.defineProperty = function(object, name, descriptor) { - var observer = new ArrayReduction(descriptor.array, descriptor.path, - descriptor.reduce, - descriptor.initial); - - Object.defineProperty(object, name, { - get: function() { - observer.deliver(); - return observer.value; - } - }); - - if (Observer.hasObjectObserve) - return observer; - - var value = observer.value; - Object.defineProperty(observer, 'value', { - get: function() { - return value; - }, - set: function(newValue) { - if (Observer.hasObjectObserve) { - Object.getNotifier(object).notify({ - object: object, - type: 'updated', - name: name, - oldValue: value - }); - } - value = newValue; - } - }) - - return observer; -}