Skip to content

Commit

Permalink
[BUGFIX release] ensure computed.sort sortProps don’t leak
Browse files Browse the repository at this point in the history 
NOTE: This mechanism of dynamically adding/remove DK’s is not public API.
  • Loading branch information
@stefanpenner
stefanpenner committed Aug 27, 2015
1 parent 2493cab commit 7b5339b
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 32 deletions.
19 changes: 19 additions & 0 deletions packages/ember-metal/lib/computed.js
View file
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
32 changes: 14 additions & 18 deletions packages/ember-runtime/lib/computed/reduce_computed_macros.js
View file
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
83 changes: 69 additions & 14 deletions packages/ember-runtime/tests/computed/reduce_computed_macros_test.js
View file
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,7 +1222,6 @@ 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() {
Expand All @@ -1233,10 +1232,10 @@ QUnit.module('sort - concurrency', {
});
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 Down Expand Up @@ -1270,15 +1269,15 @@ QUnit.test('sort correctly after mutation to the sort', function() {
QUnit.test('sort correctly on multiple instances of the same class', function() {
var obj2 = klass.create({
items: Ember.A([
{ name: 'W', count: 23 },
{ name: 'X', count: 24 },
{ name: 'Y', count: 25 },
{ name: 'Z', count: 26 }
{ 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(obj.get('sortedItems').mapBy('name'), ['A', 'B', 'C', 'D'], 'initial');
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);
Expand All @@ -1287,6 +1286,62 @@ QUnit.test('sort correctly on multiple instances of the same class', function()

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', {
Expand Down

0 comments on commit 7b5339b

Please sign in to comment.