Skip to content

Commit

Permalink
Merge pull request #12991 from mmun/fix-computed-sort
Browse files Browse the repository at this point in the history 
[BUGFIX beta] Fix a regression in Ember.computed.sort
  • Loading branch information
@krisselden
krisselden committed Feb 21, 2016
2 parents b00c076 + f05bd2d commit 4a404de
Show file tree
Hide file tree
Showing 2 changed files with 150 additions and 48 deletions.
94 changes: 58 additions & 36 deletions packages/ember-runtime/lib/computed/reduce_computed_macros.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { isArray } from 'ember-runtime/utils';
import { A as emberA } from 'ember-runtime/system/native_array';
import isNone from 'ember-metal/is_none';
import getProperties from 'ember-metal/get_properties';
import WeakMap from 'ember-metal/weak_map';

function reduceMacro(dependentKey, callback, initialValue) {
return computed(`${dependentKey}.[]`, function() {
Expand Down Expand Up @@ -608,53 +609,74 @@ function customSort(itemsKey, comparator) {
// 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);
}
let cp = new ComputedProperty(function(key) {
let itemsKeyIsAtThis = (itemsKey === '@this');
let sortProperties = get(this, sortPropertiesKey);

assert(
`The sort definition for '${key}' on ${this} must be a function or an array of strings`,
isArray(sortProperties) && sortProperties.every(s => typeof s === 'string')
);

var items = itemsKey === '@this' ? this : get(this, itemsKey);
var sortProperties = get(this, sortPropertiesKey);
let normalizedSortProperties = normalizeSortProperties(sortProperties);

if (items === null || typeof items !== 'object') { return emberA(); }
// Add/remove property observers as required.
let activeObserversMap = cp._activeObserverMap || (cp._activeObserverMap = new WeakMap());
let activeObservers = activeObserversMap.get(this);

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

cp._sortPropObservers = [];
function sortPropertyDidChange() {
this.notifyPropertyChange(key);
}

if (!isArray(sortProperties)) { return items; }
activeObservers = normalizedSortProperties.map(([prop]) => {
let path = itemsKeyIsAtThis ? `@each.${prop}` : `${itemsKey}.@each.${prop}`;
let args = [this, path, sortPropertyDidChange];
addObserver.apply(null, args);
return args;
});

// Normalize properties
var normalizedSort = sortProperties.map(p => {
let [prop, direction] = p.split(':');
direction = direction || 'asc';
activeObserversMap.set(this, activeObservers);

return [prop, direction];
});
// Sort and return the array.
let items = itemsKeyIsAtThis ? this : get(this, itemsKey);

// 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);
});
if (isArray(items)) {
return sortByNormalizedSortProperties(items, normalizedSortProperties);
} else {
return emberA();
}
});

return emberA(items.slice().sort((itemA, itemB) => {
for (var i = 0; i < normalizedSort.length; ++i) {
var [prop, direction] = normalizedSort[i];
var result = compare(get(itemA, prop), get(itemB, prop));
if (result !== 0) {
return (direction === 'desc') ? (-1 * result) : result;
}
}
cp._activeObserverMap = undefined;

return 0;
}));
return cp.property(`${sortPropertiesKey}.[]`).readOnly();
}

function normalizeSortProperties(sortProperties) {
return sortProperties.map(p => {
let [prop, direction] = p.split(':');
direction = direction || 'asc';

return [prop, direction];
});
}

function sortByNormalizedSortProperties(items, normalizedSortProperties) {
return emberA(items.slice().sort((itemA, itemB) => {
for (let i = 0; i < normalizedSortProperties.length; i++) {
let [prop, direction] = normalizedSortProperties[i];
let result = compare(get(itemA, prop), get(itemB, prop));
if (result !== 0) {
return (direction === 'desc') ? (-1 * result) : result;
}
}

return cp.property(`${itemsKey}.[]`, `${sortPropertiesKey}.[]`).readOnly();
return 0;
}));
}
104 changes: 92 additions & 12 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 @@ -5,6 +5,7 @@ import ObjectProxy from 'ember-runtime/system/object_proxy';
import { get } from 'ember-metal/property_get';
import { set } from 'ember-metal/property_set';
import { addObserver } from 'ember-metal/observer';
import { computed } from 'ember-metal/computed';
import { observer } from 'ember-metal/mixin';
import {
sum,
Expand Down Expand Up @@ -1203,10 +1204,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 @@ -1223,19 +1224,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('[email protected]', (a, b) => a.count - b.count)
}).create({
});
obj = klass.create({
items: emberA([
{ 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 @@ -1255,7 +1257,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 @@ -1266,6 +1268,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: emberA([
{ 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: computed('sibling.sortedItems.[]', function() {
return this.get('sibling.sortedItems');
}),
asdf: observer('sibling.sortedItems.[]', function() {
this.get('sibling.sortedItems');
})
}).create({
sibling: obj
});

/*
┌───────────┐ ┌────────────┐
│sortedProps│ │sortedProps2│
└───────────┘ └────────────┘
▲ ▲
│ ╔═══════════╗ │
│─ ─ ─ ─ ─ ─ ─ ▶║ CP (sort) ║◀─ ─ ─ ─ ─ ─ ─ ┤
│ ╚═══════════╝ │
│ │
┌───────────┐ ┏━━━━━━━━━━━┓ ┏━━━━━━━━━━━━┓
│ │ ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃ ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┃ ┃
│ items │◀── [email protected] │◀──┃sortedItems┃◀─── [email protected] │◀───┃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

0 comments on commit 4a404de

Please sign in to comment.