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.

Conflicts:
	packages/ember-metal/lib/chains.js
	packages/ember-metal/lib/computed.js

Cherry-picked from 4678ce9
  • Loading branch information
krisselden authored and ef4 committed Aug 10, 2015
1 parent 689318b commit e25b57c
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 96 deletions.
10 changes: 3 additions & 7 deletions packages/ember-metal/lib/chains.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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];
}
Expand Down
195 changes: 110 additions & 85 deletions packages/ember-metal/lib/computed.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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() {
Expand All @@ -151,7 +157,7 @@ var ComputedPropertyPrototype = ComputedProperty.prototype;
@public
*/
ComputedPropertyPrototype.volatile = function() {
this._cacheable = false;
this._volatile = true;
return this;
};

Expand Down Expand Up @@ -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);
}
};

Expand Down Expand Up @@ -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;
};

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

Expand Down
2 changes: 1 addition & 1 deletion packages/ember-views/lib/mixins/view_context_support.js
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
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

2 comments on commit e25b57c

@denzo
Copy link

@denzo denzo commented on e25b57c Aug 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used "volatile" after using a computed property with a new getter and setter and noticing that the "get" is not called after the "set" was called. Which means the value was always the same.

Is it now fixed and I can remove "volatile"?

@stefanpenner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it now fixed and I can remove "volatile"?

You should provide an running example of your problem, its tricky to be sure interpretations of your written problem are correct.

Please sign in to comment.