Skip to content

Commit

Permalink
[BUGFIX beta] Cleanup CP set, volatile and various related things.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@krisselden
krisselden committed Aug 5, 2015
1 parent 41ece2b commit 4678ce9
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 95 deletions.
10 changes: 3 additions & 7 deletions packages/ember-metal/lib/chains.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,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 = new EmptyObject();

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 = new EmptyObject();
}

ChainWatchers.prototype = {
Expand Down Expand Up @@ -327,7 +323,7 @@ ChainNode.prototype = {
var chains = this._chains;
var node;
if (chains === undefined) {
chains = this._chains = new Chains();
chains = this._chains = new EmptyObject();
} else {
node = chains[key];
}
Expand Down
188 changes: 104 additions & 84 deletions packages/ember-metal/lib/computed.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,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;
}
Expand All @@ -135,6 +135,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() {
Expand All @@ -149,7 +155,7 @@ var ComputedPropertyPrototype = ComputedProperty.prototype;
@public
*/
ComputedPropertyPrototype.volatile = function() {
this._cacheable = false;
this._volatile = true;
return this;
};

Expand Down Expand Up @@ -259,17 +265,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) {
let meta = metaFor(obj);
let cache = meta.readableCache();
if (cache && cache[keyName] !== undefined) {
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.readableCache();
if (cache && cache[keyName] !== undefined) {
cache[keyName] = undefined;
removeDependentKeys(this, obj, keyName, meta);
}
};

Expand Down Expand Up @@ -301,35 +314,33 @@ ComputedPropertyPrototype.didChange = function(obj, keyName) {
@public
*/
ComputedPropertyPrototype.get = function(obj, keyName) {
var ret, cache, meta;
if (this._cacheable) {
meta = metaFor(obj);
cache = meta.writableCache();

var result = cache[keyName];

if (result === UNDEFINED) {
return undefined;
} else if (result !== undefined) {
return result;
}
if (this._volatile) {
return this._getter.call(obj, keyName);
}

ret = this._getter.call(obj, keyName);
let meta = metaFor(obj);
let cache = meta.writableCache();

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;
}

let chainWatchers = meta.readableChainWatchers();
if (chainWatchers) {
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.readableChainWatchers();
if (chainWatchers) {
chainWatchers.revalidate(keyName);
}
addDependentKeys(this, obj, keyName, meta);

return ret;
};

Expand Down Expand Up @@ -382,49 +393,65 @@ 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);
}

try {
this._set(obj, keyName, value);
} finally {
this._suspended = oldSuspended;
if (this._volatile) {
return this.volatileSet(obj, keyName, value);
}

return this.setWithSuspend(obj, keyName, value);
};

ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, value) {
var cacheable = this._cacheable;
var setter = this._setter;
var meta = metaFor(obj, cacheable);
var cache = meta.readableCache();
var hadCachedValue = false;
ComputedPropertyPrototype.clobberSet = function computedPropertyClobberSet(obj, keyName, value) {
let cachedValue = cacheFor(obj, keyName);
defineProperty(obj, keyName, null, cachedValue);
set(obj, keyName, value);
return value;
};

var cachedValue, ret;
ComputedPropertyPrototype.volatileSet = function computedPropertyVolatileSet(obj, keyName, value) {
return this._setter.call(obj, keyName, value);
};

if (this._readOnly) {
throw new EmberError(`Cannot set read-only property "${keyName}" on object: ${inspect(obj)}`);
ComputedPropertyPrototype.setWithSuspend = function computedPropertySetWithSuspend(obj, keyName, value) {
let oldSuspended = this._suspended;
this._suspended = obj;
try {
return this._set(obj, keyName, value);
} finally {
this._suspended = oldSuspended;
}
};

if (cacheable && cache && cache[keyName] !== undefined) {
ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, value) {
// cache requires own meta
let meta = metaFor(obj);
// either there is a writable cache or we need one to update
let cache = meta.writableCache();
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.peekWatching(keyName);
let watched = meta.peekWatching(keyName);
if (watched) {
propertyWillChange(obj, keyName);
}
Expand All @@ -433,18 +460,14 @@ ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, valu
cache[keyName] = undefined;
}

if (cacheable) {
if (!hadCachedValue) {
addDependentKeys(this, obj, keyName, meta);
}
if (!cache) {
cache = meta.writableCache();
}
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) {
Expand All @@ -456,20 +479,17 @@ ComputedPropertyPrototype._set = function computedPropertySet(obj, keyName, valu

/* called before property is overridden */
ComputedPropertyPrototype.teardown = function(obj, keyName) {
var meta = metaFor(obj);
if (this._volatile) {
return;
}
let meta = metaFor(obj);
let cache = meta.readableCache();
if (cache) {
if (keyName in cache) {
removeDependentKeys(this, obj, keyName, meta);
}

if (this._cacheable) { delete cache[keyName]; }
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
Expand Down Expand Up @@ -556,8 +576,8 @@ export default function computed(func) {
@public
*/
function cacheFor(obj, key) {
var meta = obj['__ember_meta__'];
var cache = meta && meta.readableCache();
var meta = obj.__ember_meta__;
var cache = meta && meta.source === obj && meta.readableCache();
var ret = cache && cache[key];

if (ret === UNDEFINED) {
Expand Down
3 changes: 3 additions & 0 deletions packages/ember-metal/tests/binding/sync_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand All @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-metal/tests/observer_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,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');

Expand Down
2 changes: 1 addition & 1 deletion packages/ember-views/lib/mixins/view_context_support.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions packages/ember-views/tests/views/view/init_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand All @@ -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);
});
Expand Down

0 comments on commit 4678ce9

Please sign in to comment.