Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for pr 12215 #12221

Closed
wants to merge 3 commits into from
Closed

Fix for pr 12215 #12221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a1ea35
Add Weakmap – Weak from Map -> Obj, not Obj -> Map
stefanpenner Aug 27, 2015
2493cab
Added a failing test for #12212. Demonstrates issues with Ember.compu…
Aug 26, 2015
7b5339b
[BUGFIX release] ensure computed.sort sortProps don’t leak
stefanpenner Aug 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/ember-metal/lib/computed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,25 @@ ComputedPropertyPrototype.meta = function(meta) {
}
};

/**
@private

@method _remove
@param keysToRemove {Array}
*/
ComputedPropertyPrototype._remove = function(_keysToRemove) {
var keysToRemove = _keysToRemove.slice();

for (var i = this._dependentKeys.length - 1; i >= 0; i--) {
for (var j = keysToRemove.length - 1; j >= 0; j--) {
if (this._dependentKeys[i] === keysToRemove[j]) {
this._dependentKeys.splice(i, 1);
keysToRemove.splice(j, 1);
}
}
}
};

// invalidate cache when CP key changes
ComputedPropertyPrototype.didChange = function(obj, keyName) {
// _suspended is set via a CP.set to ensure we don't clear
Expand Down
14 changes: 14 additions & 0 deletions packages/ember-metal/lib/meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import EmptyObject from 'ember-metal/empty_object';
*/
let members = {
cache: ownMap,
weak: ownMap,
watching: inheritedMap,
mixins: inheritedMap,
bindings: inheritedMap,
Expand All @@ -53,6 +54,7 @@ function Meta(obj, parentMeta) {
this._deps = undefined;
this._chainWatchers = undefined;
this._chains = undefined;
this._weak = undefined;
// used only internally
this.source = obj;

Expand Down Expand Up @@ -148,6 +150,18 @@ Meta.prototype._getInherited = function(key) {
}
};

Meta.prototype.getWeak = function(symbol) {
var weak = this.readableWeak();
if (weak) {
return weak[symbol];
}
};

Meta.prototype.setWeak = function(symbol, value) {
var weak = this.writableWeak();
return weak[symbol] = value;
};

Meta.prototype._findInherited = function(key, subkey) {
let pointer = this;
while (pointer !== undefined) {
Expand Down
39 changes: 39 additions & 0 deletions packages/ember-metal/lib/weak_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { GUID_KEY } from 'ember-metal/utils';
import { meta } from 'ember-metal/meta';

var id = 0;

/*
* @private
* @class Ember.WeakMap
*
* Weak relationship from Map -> Key, but not Key to Map.
*
* Key must be a non null object
*/
export default function WeakMap() {
this._id = GUID_KEY + (id++);
}

/*
* @method get
* @param key {Object}
* @return {*} stored value
*/
WeakMap.prototype.get = function(obj) {
var map = meta(obj).readableWeak();
if (map) {
return map[this._id];
}
};

/*
* @method set
* @param key {Object}
* @param value {Any}
* @return {Any} stored value
*/
WeakMap.prototype.set = function(obj, value) {
meta(obj).writableWeak()[this._id] = value;
return this;
};
48 changes: 48 additions & 0 deletions packages/ember-metal/tests/weak_map_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import WeakMap from 'ember-metal/weak_map';

QUnit.module('Ember.WeakMap');

QUnit.test('has weakMap like qualities', function(assert) {
var map = new WeakMap();
var map2 = new WeakMap();

var a = {};
var b = {};
var c = {};

equal(map.get(a), undefined);
equal(map.get(b), undefined);
equal(map.get(c), undefined);

equal(map2.get(a), undefined);
equal(map2.get(b), undefined);
equal(map2.get(c), undefined);

equal(map.set(a, 1), map, 'map.set should return itself');
equal(map.get(a), 1);
equal(map.set(b, undefined), map);
equal(map.set(a, 2), map);
equal(map.get(a), 2);
equal(map.set(b, undefined), map);

equal(map2.get(a), undefined);
equal(map2.get(b), undefined);
equal(map2.get(c), undefined);

equal(map.set(c, 1), map);
equal(map.get(c), 1);
equal(map.get(a), 2);
equal(map.get(b), undefined);

equal(map2.set(a, 3), map2);
equal(map2.set(b, 4), map2);
equal(map2.set(c, 5), map2);

equal(map2.get(a), 3);
equal(map2.get(b), 4);
equal(map2.get(c), 5);

equal(map.get(c), 1);
equal(map.get(a), 2);
equal(map.get(b), undefined);
});
32 changes: 14 additions & 18 deletions packages/ember-runtime/lib/computed/reduce_computed_macros.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import { assert } from 'ember-metal/debug';
import { get } from 'ember-metal/property_get';
import EmberError from 'ember-metal/error';
import { ComputedProperty, computed } from 'ember-metal/computed';
import { addObserver, removeObserver } from 'ember-metal/observer';
import compare from 'ember-runtime/compare';
import { isArray } from 'ember-runtime/utils';
import { symbol } from 'ember-metal/utils';

function reduceMacro(dependentKey, callback, initialValue) {
return computed(`${dependentKey}.[]`, function() {
Expand Down Expand Up @@ -559,26 +559,22 @@ function customSort(itemsKey, comparator) {
return value.slice().sort((x, y) => comparator.call(this, x, y));
});
}

// This one needs to dynamically set up and tear down observers on the itemsKey
// depending on the sortProperties
function propertySort(itemsKey, sortPropertiesKey) {
var cp = new ComputedProperty(function(key) {
function didChange() {
this.notifyPropertyChange(key);
}
var sym = symbol('sortDependentKeys');

var cp = new ComputedProperty(function(key) {
var items = itemsKey === '@this' ? this : get(this, itemsKey);
var sortProperties = get(this, sortPropertiesKey);

if (items === null || typeof items !== 'object') { return Ember.A(); }

// TODO: Ideally we'd only do this if things have changed
if (cp._sortPropObservers) {
cp._sortPropObservers.forEach(args => removeObserver.apply(null, args));
}
var sortDependentKeys = cp[sym];

cp._sortPropObservers = [];
if (sortDependentKeys) {
cp._remove(sortDependentKeys);
}

if (!isArray(sortProperties)) { return items; }

Expand All @@ -590,13 +586,13 @@ function propertySort(itemsKey, sortPropertiesKey) {
return [prop, direction];
});

// TODO: Ideally we'd only do this if things have changed
// Add observers
normalizedSort.forEach(prop => {
var args = [this, `${itemsKey}.@each.${prop[0]}`, didChange];
cp._sortPropObservers.push(args);
addObserver.apply(null, args);
});
sortDependentKeys = normalizedSort.map(prop => `${itemsKey}.@each.${prop[0]}`);

cp[sym] = sortDependentKeys;

// NOTE: This mechanism of dynamically adding/remove DK’s is not public API.
// I is likely something that should eventually be explored,
cp._dependentKeys.push.apply(cp._dependentKeys, sortDependentKeys);

return Ember.A(items.slice().sort((itemA, itemB) => {
for (var i = 0; i < normalizedSort.length; ++i) {
Expand Down
103 changes: 91 additions & 12 deletions packages/ember-runtime/tests/computed/reduce_computed_macros_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,10 +1202,10 @@ QUnit.module('sort - stability', {
sortedItems: sort('items', 'sortProps')
}).create({
items: [
{ name: 'A', count: 1 },
{ name: 'B', count: 1 },
{ name: 'C', count: 1 },
{ name: 'D', count: 1 }
{ name: 'A', count: 1, thing: 4 },
{ name: 'B', count: 1, thing: 3 },
{ name: 'C', count: 1, thing: 2 },
{ name: 'D', count: 1, thing: 4 }
]
});
},
Expand All @@ -1222,19 +1222,20 @@ QUnit.test('sorts correctly as only one property changes', function() {
deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'final');
});


var klass;
QUnit.module('sort - concurrency', {
setup() {
obj = EmberObject.extend({
klass = EmberObject.extend({
sortProps: ['count'],
sortedItems: sort('items', 'sortProps'),
customSortedItems: sort('items.@each.count', (a, b) => a.count - b.count)
}).create({
});
obj = klass.create({
items: Ember.A([
{ name: 'A', count: 1 },
{ name: 'B', count: 2 },
{ name: 'C', count: 3 },
{ name: 'D', count: 4 }
{ name: 'A', count: 1, thing: 4, id: 1 },
{ name: 'B', count: 2, thing: 3, id: 2 },
{ name: 'C', count: 3, thing: 2, id: 3 },
{ name: 'D', count: 4, thing: 1, id: 4 }
])
});
},
Expand All @@ -1254,7 +1255,7 @@ QUnit.test('sorts correctly after mutation to the sort properties', function() {
deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'D', 'B', 'C'], 'final');
});

QUnit.test('sort correctl after mutation to the sor ', function() {
QUnit.test('sort correctly after mutation to the sort', function() {
deepEqual(obj.get('customSortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'initial');

set(obj.get('items')[1], 'count', 5);
Expand All @@ -1265,6 +1266,84 @@ QUnit.test('sort correctl after mutation to the sor ', function() {
deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'D', 'B', 'C'], 'final');
});

QUnit.test('sort correctly on multiple instances of the same class', function() {
var obj2 = klass.create({
items: Ember.A([
{ name: 'W', count: 23, thing: 4 },
{ name: 'X', count: 24, thing: 3 },
{ name: 'Y', count: 25, thing: 2 },
{ name: 'Z', count: 26, thing: 1 }
])
});

deepEqual(obj2.get('sortedItems').mapBy('name'), ['W', 'X', 'Y', 'Z'], 'initial');
deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'initial');

set(obj.get('items')[1], 'count', 5);
set(obj.get('items')[2], 'count', 6);
set(obj2.get('items')[1], 'count', 27);
set(obj2.get('items')[2], 'count', 28);

deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'D', 'B', 'C'], 'final');
deepEqual(obj2.get('sortedItems').mapBy('name'), ['W', 'Z', 'X', 'Y'], 'final');

obj.set('sortProps', ['thing']);

deepEqual(obj.get('sortedItems').mapBy('name'), ['D', 'C', 'B', 'A'], 'final');

obj2.notifyPropertyChange('sortedItems'); // invalidate to flush, to get DK refreshed
obj2.get('sortedItems'); // flush to get updated DK

obj2.set('items.firstObject.count', 9999);

deepEqual(obj2.get('sortedItems').mapBy('name'), ['Z', 'X', 'Y', 'W'], 'final');
});


QUnit.test('sort correctly when multiple sorts are chained on the same instance of a class', function() {
var obj2 = klass.extend({
items: Ember.computed('sibling.sortedItems.[]', function() {
return this.get('sibling.sortedItems');
}),
asdf: Ember.observer('sibling.sortedItems.[]', function() {
this.get('sibling.sortedItems');
})
}).create({
sibling: obj
});

/*
┌───────────┐ ┌────────────┐
│sortedProps│ │sortedProps2│
└───────────┘ └────────────┘
▲ ▲
│ ╔═══════════╗ │
│─ ─ ─ ─ ─ ─ ─ ▶║ CP (sort) ║◀─ ─ ─ ─ ─ ─ ─ ┤
│ ╚═══════════╝ │
│ │
┌───────────┐ ┏━━━━━━━━━━━┓ ┏━━━━━━━━━━━━┓
│ │ ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃ ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃
│ items │◀── items.@each.count │◀──┃sortedItems┃◀─── items.@each.count │◀───┃sortedItems2┃
│ │ └ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃ └ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃
└───────────┘ ┗━━━━━━━━━━━┛ ┗━━━━━━━━━━━━┛
*/

deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'obj.sortedItems.name should be sorted alpha');
deepEqual(obj2.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'obj2.sortedItems.name should be sorted alpha');

set(obj.get('items')[1], 'count', 5);
set(obj.get('items')[2], 'count', 6);

deepEqual(obj.get('sortedItems').mapBy('name'), ['A', 'D', 'B', 'C'], 'obj.sortedItems.name should now have changed');
deepEqual(obj2.get('sortedItems').mapBy('name'), ['A', 'D', 'B', 'C'], 'obj2.sortedItems.name should still mirror sortedItems2');

obj.set('sortProps', ['thing']);
obj2.set('sortProps', ['id']);

deepEqual(obj2.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'we now sort obj2 by id, so we expect a b c d');
deepEqual(obj.get('sortedItems').mapBy('name'), ['D', 'C', 'B', 'A'], 'we now sort obj by thing');
});

QUnit.module('max', {
setup() {
obj = EmberObject.extend({
Expand Down