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.

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

[BUGFIX beta] fix for lastObject/firstObject update issue #15510

Merged
merged 1 commit into from
Jul 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 22 additions & 12 deletions packages/ember-runtime/lib/mixins/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,19 +141,29 @@ export function arrayContentDidChange(array, startIdx, removeAmt, addAmt) {

let meta = peekMeta(array);
let cache = meta && meta.readableCache();

if (cache) {
if (cache.firstObject !== undefined &&
objectAt(array, 0) !== cacheFor.get(cache, 'firstObject')) {
propertyWillChange(array, 'firstObject', meta);
propertyDidChange(array, 'firstObject', meta);
}
if (cache.lastObject !== undefined &&
objectAt(array, get(array, 'length') - 1) !== cacheFor.get(cache, 'lastObject')) {
propertyWillChange(array, 'lastObject', meta);
propertyDidChange(array, 'lastObject', meta);
}
if (cache !== undefined) {
let length = get(array, 'length');
let addedAmount = (addAmt === -1 ? 0 : addAmt);
let removedAmount = (removeAmt === -1 ? 0 : removeAmt);
let delta = addedAmount - removedAmount;
let previousLength = length - delta;

let normalStartIdx = startIdx < 0 ? previousLength + startIdx : startIdx;
Copy link
Member

Choose a reason for hiding this comment

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

👍

if (cache.firstObject !== undefined && normalStartIdx === 0) {
propertyWillChange(array, 'firstObject');
propertyDidChange(array, 'firstObject');
}

if (cache.lastObject !== undefined) {
let previousLastIndex = previousLength - 1;
let lastAffectedIndex = normalStartIdx + removedAmount;
if (previousLastIndex < lastAffectedIndex) {
propertyWillChange(array, 'lastObject');
propertyDidChange(array, 'lastObject');
}
}
}

return array;
}

Expand Down
10 changes: 10 additions & 0 deletions packages/ember-runtime/tests/suites/mutable_array/insertAt.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,20 @@ suite.test('[A,B,C].insertAt(1,X) => [A,X,B,C] + notify', function() {
let after = [before[0], item, before[1], before[2]];
let obj = this.newObject(before);
let observer = this.newObserver(obj, '[]', '@each', 'length', 'firstObject', 'lastObject');
let objectAtCalls = [];

let objectAt = obj.objectAt;
obj.objectAt = (ix) => {
objectAtCalls.push(ix);
return objectAt.call(obj, ix);
}

obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */

objectAtCalls.splice(0, objectAtCalls.length);

obj.insertAt(1, item);
deepEqual(objectAtCalls, [], 'objectAt is not called when only inserting items');

deepEqual(this.toArray(obj), after, 'post item results');
equal(get(obj, 'length'), after.length, 'length');
Expand Down
22 changes: 22 additions & 0 deletions packages/ember-runtime/tests/suites/mutable_array/pushObject.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,26 @@ suite.test('[A,B,C].pushObject(X) => [A,B,C,X] + notify', function() {
equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject');
});

suite.test('[A,B,C,C].pushObject(A) => [A,B,C,C] + notify', function() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

We'll need a test for removing the tail from a negative index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added that, you could take another shot when you have time, thanks

let before = this.newFixture(3);
let item = before[2]; // note same object as current tail. should end up twice
let after = [before[0], before[1], before[2], item];
let obj = this.newObject(before);
let observer = this.newObserver(obj, '[]', '@each', 'length', 'firstObject', 'lastObject');

obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */

obj.pushObject(item);

deepEqual(this.toArray(obj), after, 'post item results');
equal(get(obj, 'length'), after.length, 'length');

equal(observer.timesCalled('[]'), 1, 'should have notified [] once');
equal(observer.timesCalled('@each'), 0, 'should not have notified @each once');
equal(observer.timesCalled('length'), 1, 'should have notified length once');

equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject');
equal(observer.validate('lastObject'), true, 'should have notified lastObject');
});

export default suite;
21 changes: 21 additions & 0 deletions packages/ember-runtime/tests/suites/mutable_array/replace.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,27 @@ suite.test('[A,B,C,D].replace(2,2) => [A,B] + notify', function() {
equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject once');
});

suite.test('[A,B,C,D].replace(-1,1) => [A,B,C] + notify', function() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

let before = this.newFixture(4);
let after = [before[0], before[1], before[2]];

let obj = this.newObject(before);
let observer = this.newObserver(obj, '[]', '@each', 'length', 'firstObject', 'lastObject');

obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */

obj.replace(-1, 1);

deepEqual(this.toArray(obj), after, 'post item results');

equal(observer.timesCalled('[]'), 1, 'should have notified [] once');
equal(observer.timesCalled('@each'), 0, 'should not have notified @each once');
equal(observer.timesCalled('length'), 1, 'should have notified length once');
equal(observer.timesCalled('lastObject'), 1, 'should have notified lastObject once');

equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject once');
});

suite.test('Adding object should notify enumerable observer', function() {
let fixtures = this.newFixture(4);
let obj = this.newObject(fixtures);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ suite.test('[A,B,C].unshiftObject(X) => [X,A,B,C] + notify', function() {
suite.test('[A,B,C].unshiftObject(A) => [A,A,B,C] + notify', function() {
let before = this.newFixture(3);
let item = before[0]; // note same object as current head. should end up twice
let after = [item, before[0], before[1], before[2]];
let after = [item, before[0], before[1], before[2]];
let obj = this.newObject(before);
let observer = this.newObserver(obj, '[]', '@each', 'length', 'firstObject', 'lastObject');

Expand All @@ -73,7 +73,7 @@ suite.test('[A,B,C].unshiftObject(A) => [A,A,B,C] + notify', function() {
equal(observer.timesCalled('@each'), 0, 'should not have notified @each once');
equal(observer.timesCalled('length'), 1, 'should have notified length once');

equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject');
equal(observer.validate('firstObject'), true, 'should have notified firstObject');
equal(observer.validate('lastObject'), false, 'should NOT have notified lastObject');
});

Expand Down