From e25b57c997a8d70f1aa49dbc8a01ece189a2d979 Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Wed, 5 Aug 2015 02:08:17 -0700 Subject: [PATCH] [BUGFIX beta] Cleanup CP set, volatile and various related things. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Breakup set method into a top level branching method that has 4 paths, readonly check/throw, clobber set, volatile set or normal set. `volatile()` does not simply mean don’t cache, it means this value is inherently not observable and/or you need to manually notify/cache the value. ArrayProxy is a good example, length is observable but has to be manually notified for NativeArray mixin, so ArrayProxy just delegates to the underlying array but doesn’t setup DKs or any notification since it is notified already by the mutation methods. It is not an escape value for you not getting the correct DKs or it seems to solve some bug I can’t figure out. It is not the equivalent of the old sprout core behavior and hasn’t been for a while. If you have a dependency that does not fit into a DK, you can notifyPropertyChange() manually. If you don’t understand the above you should not be using `volatile` it has an extremely narrow use case. Conflicts: packages/ember-metal/lib/chains.js packages/ember-metal/lib/computed.js Cherry-picked from 4678ce908dff026c2396bfd5ffabc4ed92ce041c --- packages/ember-metal/lib/chains.js | 10 +- packages/ember-metal/lib/computed.js | 195 ++++++++++-------- .../ember-metal/tests/binding/sync_test.js | 3 + packages/ember-metal/tests/observer_test.js | 2 +- .../lib/mixins/view_context_support.js | 2 +- .../ember-views/tests/views/view/init_test.js | 4 +- 6 files changed, 120 insertions(+), 96 deletions(-) diff --git a/packages/ember-metal/lib/chains.js b/packages/ember-metal/lib/chains.js index 395f5bbaf26..2f070135dd5 100644 --- a/packages/ember-metal/lib/chains.js +++ b/packages/ember-metal/lib/chains.js @@ -14,20 +14,16 @@ function isObject(obj) { } function isVolatile(obj) { - return !(isObject(obj) && obj.isDescriptor && obj._cacheable); + return !(isObject(obj) && obj.isDescriptor && !obj._volatile); } -function Chains() { } - -Chains.prototype = Object.create(null); - function ChainWatchers(obj) { // this obj would be the referencing chain node's parent node's value this.obj = obj; // chain nodes that reference a key in this obj by key // we only create ChainWatchers when we are going to add them // so create this upfront - this.chains = new Chains(); + this.chains = {}; } ChainWatchers.prototype = { @@ -319,7 +315,7 @@ ChainNode.prototype = { var chains = this._chains; var node; if (chains === undefined) { - chains = this._chains = new Chains(); + chains = this._chains = {}; } else { node = chains[key]; } diff --git a/packages/ember-metal/lib/computed.js b/packages/ember-metal/lib/computed.js index 03254c1d6a4..ddb023ff6db 100644 --- a/packages/ember-metal/lib/computed.js +++ b/packages/ember-metal/lib/computed.js @@ -124,7 +124,7 @@ function ComputedProperty(config, opts) { this._dependentKeys = undefined; this._suspended = undefined; this._meta = undefined; - this._cacheable = true; + this._volatile = false; this._dependentKeys = opts && opts.dependentKeys; this._readOnly = false; } @@ -137,6 +137,12 @@ var ComputedPropertyPrototype = ComputedProperty.prototype; Call on a computed property to set it into non-cached mode. When in this mode the computed property will not automatically cache the return value. + It also does not automatically fire any change events. You must manually notify + any changes if you want to observe this property. + + Dependency keys have no effect on volatile properties as they are for cache + invalidation and notification when cached value is invalidated. + ```javascript var outsideService = Ember.Object.extend({ value: function() { @@ -151,7 +157,7 @@ var ComputedPropertyPrototype = ComputedProperty.prototype; @public */ ComputedPropertyPrototype.volatile = function() { - this._cacheable = false; + this._volatile = true; return this; }; @@ -266,16 +272,24 @@ ComputedPropertyPrototype.meta = function(meta) { } }; -/* impl descriptor API */ +// invalidate cache when CP key changes ComputedPropertyPrototype.didChange = function(obj, keyName) { // _suspended is set via a CP.set to ensure we don't clear // the cached value set by the setter - if (this._cacheable && this._suspended !== obj) { - var meta = metaFor(obj); - if (meta.cache && meta.cache[keyName] !== undefined) { - meta.cache[keyName] = undefined; - removeDependentKeys(this, obj, keyName, meta); - } + if (this._volatile || this._suspended === obj) { + return; + } + + // don't create objects just to invalidate + let meta = obj.__ember_meta__; + if (meta.source !== obj) { + return; + } + + let cache = meta.cache; + if (cache && cache[keyName] !== undefined) { + cache[keyName] = undefined; + removeDependentKeys(this, obj, keyName, meta); } }; @@ -307,37 +321,36 @@ ComputedPropertyPrototype.didChange = function(obj, keyName) { @public */ ComputedPropertyPrototype.get = function(obj, keyName) { - var ret, cache, meta; - if (this._cacheable) { - meta = metaFor(obj); - cache = meta.cache; - - var result = cache && cache[keyName]; + if (this._volatile) { + return this._getter.call(obj, keyName); + } - if (result === UNDEFINED) { - return undefined; - } else if (result !== undefined) { - return result; - } + let meta = metaFor(obj); + let cache = meta.cache; + if (!cache) { + cache = meta.cache = {}; + } - ret = this._getter.call(obj, keyName); - cache = meta.cache; - if (!cache) { - cache = meta.cache = {}; - } - if (ret === undefined) { - cache[keyName] = UNDEFINED; - } else { - cache[keyName] = ret; - } + let result = cache[keyName]; + if (result === UNDEFINED) { + return undefined; + } else if (result !== undefined) { + return result; + } - if (meta.chainWatchers) { - meta.chainWatchers.revalidate(keyName); - } - addDependentKeys(this, obj, keyName, meta); + let ret = this._getter.call(obj, keyName); + if (ret === undefined) { + cache[keyName] = UNDEFINED; } else { - ret = this._getter.call(obj, keyName); + cache[keyName] = ret; } + + let chainWatchers = meta.chainWatchers; + if (chainWatchers) { + chainWatchers.revalidate(keyName); + } + addDependentKeys(this, obj, keyName, meta); + return ret; }; @@ -390,49 +403,68 @@ ComputedPropertyPrototype.get = function(obj, keyName) { @return {Object} The return value of the function backing the CP. @public */ -ComputedPropertyPrototype.set = function computedPropertySetWithSuspend(obj, keyName, value) { - var oldSuspended = this._suspended; +ComputedPropertyPrototype.set = function computedPropertySetEntry(obj, keyName, value) { + if (this._readOnly) { + throw new EmberError(`Cannot set read-only property "${keyName}" on object: ${inspect(obj)}`); + } - this._suspended = obj; + if (!this._setter) { + return this.clobberSet(obj, keyName, value); + } + + if (this._volatile) { + return this.volatileSet(obj, keyName, value); + } + + return this.setWithSuspend(obj, keyName, value); +}; + +ComputedPropertyPrototype.clobberSet = function computedPropertyClobberSet(obj, keyName, value) { + let cachedValue = cacheFor(obj, keyName); + defineProperty(obj, keyName, null, cachedValue); + set(obj, keyName, value); + return value; +}; + +ComputedPropertyPrototype.volatileSet = function computedPropertyVolatileSet(obj, keyName, value) { + return this._setter.call(obj, keyName, value); +}; +ComputedPropertyPrototype.setWithSuspend = function computedPropertySetWithSuspend(obj, keyName, value) { + let oldSuspended = this._suspended; + this._suspended = obj; try { - this._set(obj, keyName, value); + return this._set(obj, keyName, value); } finally { this._suspended = oldSuspended; } }; ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, value) { - var cacheable = this._cacheable; - var setter = this._setter; - var meta = metaFor(obj, cacheable); - var cache = meta.cache; - var hadCachedValue = false; - - var cachedValue, ret; - - if (this._readOnly) { - throw new EmberError(`Cannot set read-only property "${keyName}" on object: ${inspect(obj)}`); + // cache requires own meta + let meta = metaFor(obj); + // either there is a writable cache or we need one to update + let cache = meta.cache; + if (!cache) { + cache = meta.cache = {}; } - - if (cacheable && cache && cache[keyName] !== undefined) { + let hadCachedValue = false; + let cachedValue; + if (cache[keyName] !== undefined) { if (cache[keyName] !== UNDEFINED) { cachedValue = cache[keyName]; } - hadCachedValue = true; } - if (!setter) { - defineProperty(obj, keyName, null, cachedValue); - return set(obj, keyName, value); - } else { - ret = setter.call(obj, keyName, value, cachedValue); - } + let ret = this._setter.call(obj, keyName, value, cachedValue); - if (hadCachedValue && cachedValue === ret) { return; } + // allows setter to return the same value that is cached already + if (hadCachedValue && cachedValue === ret) { + return ret; + } - var watched = meta.watching[keyName]; + let watched = meta.watching && meta.watching[keyName]; if (watched) { propertyWillChange(obj, keyName); } @@ -441,18 +473,14 @@ ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, valu cache[keyName] = undefined; } - if (cacheable) { - if (!hadCachedValue) { - addDependentKeys(this, obj, keyName, meta); - } - if (!cache) { - cache = meta.cache = {}; - } - if (ret === undefined) { - cache[keyName] = UNDEFINED; - } else { - cache[keyName] = ret; - } + if (!hadCachedValue) { + addDependentKeys(this, obj, keyName, meta); + } + + if (ret === undefined) { + cache[keyName] = UNDEFINED; + } else { + cache[keyName] = ret; } if (watched) { @@ -464,20 +492,17 @@ ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, valu /* called before property is overridden */ ComputedPropertyPrototype.teardown = function(obj, keyName) { - var meta = metaFor(obj); - - if (meta.cache) { - if (keyName in meta.cache) { - removeDependentKeys(this, obj, keyName, meta); - } - - if (this._cacheable) { delete meta.cache[keyName]; } + if (this._volatile) { + return; + } + let meta = metaFor(obj); + let cache = meta.cache; + if (cache && cache[keyName] !== undefined) { + removeDependentKeys(this, obj, keyName, meta); + cache[keyName] = undefined; } - - return null; // no value to restore }; - /** This helper returns a new property descriptor that wraps the passed computed property function. You can use this helper to define properties @@ -564,8 +589,8 @@ export default function computed(func) { @public */ function cacheFor(obj, key) { - var meta = obj['__ember_meta__']; - var cache = meta && meta.cache; + var meta = obj.__ember_meta__; + var cache = meta && meta.source === obj && meta.cache; var ret = cache && cache[key]; if (ret === UNDEFINED) { diff --git a/packages/ember-metal/tests/binding/sync_test.js b/packages/ember-metal/tests/binding/sync_test.js index 9bba4cea999..83724de6e4b 100644 --- a/packages/ember-metal/tests/binding/sync_test.js +++ b/packages/ember-metal/tests/binding/sync_test.js @@ -6,6 +6,7 @@ import { import { bind } from 'ember-metal/binding'; import { computed } from 'ember-metal/computed'; import { defineProperty } from 'ember-metal/properties'; +import { propertyWillChange, propertyDidChange } from 'ember-metal/property_events'; QUnit.module('system/binding/sync_test.js'); @@ -24,7 +25,9 @@ testBoth('bindings should not sync twice in a single run loop', function(get, se }, set: function(key, value) { setCalled++; + propertyWillChange(this, key); setValue = value; + propertyDidChange(this, key); return value; } }).volatile()); diff --git a/packages/ember-metal/tests/observer_test.js b/packages/ember-metal/tests/observer_test.js index 548bab5eb7d..2b2ce9591bc 100644 --- a/packages/ember-metal/tests/observer_test.js +++ b/packages/ember-metal/tests/observer_test.js @@ -837,7 +837,7 @@ testBoth('depending on a chain with a computed property', function (get, set) { changed++; }); - equal(undefined, cacheFor(obj, 'computed'), 'addObserver should not compute CP'); + equal(cacheFor(obj, 'computed'), undefined, 'addObserver should not compute CP'); set(obj, 'computed.foo', 'baz'); diff --git a/packages/ember-views/lib/mixins/view_context_support.js b/packages/ember-views/lib/mixins/view_context_support.js index f01bc8995f3..0b69b9935fa 100644 --- a/packages/ember-views/lib/mixins/view_context_support.js +++ b/packages/ember-views/lib/mixins/view_context_support.js @@ -32,7 +32,7 @@ var ViewContextSupport = Mixin.create(LegacyViewSupport, { set(this, '_context', value); return value; } - }).volatile(), + }), /** Private copy of the view's template context. This can be set directly diff --git a/packages/ember-views/tests/views/view/init_test.js b/packages/ember-views/tests/views/view/init_test.js index bd86c589281..ab28f36e450 100644 --- a/packages/ember-views/tests/views/view/init_test.js +++ b/packages/ember-views/tests/views/view/init_test.js @@ -37,7 +37,7 @@ QUnit.test('should warn if a computed property is used for classNames', function elementId: 'test', classNames: computed(function() { return ['className']; - }).volatile() + }) }).create(); }, /Only arrays of static class strings.*For dynamic classes/i); }); @@ -48,7 +48,7 @@ QUnit.test('should warn if a non-array is used for classNameBindings', function( elementId: 'test', classNameBindings: computed(function() { return ['className']; - }).volatile() + }) }).create(); }, /Only arrays are allowed/i); });