From 0eeff7489c2abb7bcd8a8223a3029cbd7eaa2e20 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Tue, 18 Oct 2016 00:48:55 +0100 Subject: [PATCH 01/15] optimise notifications when has-many array is changed --- addon/-private/system/many-array.js | 40 +- .../records/relationship-changes-test.js | 358 ++++++++++++++++++ 2 files changed, 390 insertions(+), 8 deletions(-) create mode 100644 tests/integration/records/relationship-changes-test.js diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 56827f6c783..cc7b8b4c10e 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -87,15 +87,39 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { ); toSet = toSet.concat(newRecords); var oldLength = this.length; - this.arrayContentWillChange(0, this.length, toSet.length); - // It’s possible the parent side of the relationship may have been unloaded by this point - if (_objectIsAlive(this)) { - this.set('length', toSet.length); + var newLength = toSet.length; + + var shortestLength = Math.min(oldLength, newLength); + + var firstChangeIndex = -1; // -1 signifies no changes + // find the first change + var currentArray = this.currentState; + for (var i=0; i Date: Fri, 21 Oct 2016 09:43:24 +0100 Subject: [PATCH 02/15] move test models into Module for speed and ES6-ify --- .gitignore | 1 + .../records/relationship-changes-test.js | 40 +++++++++---------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 11c4f19a7f7..5b2f5ac81ef 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,4 @@ node_modules/ bower_components/ .metadata_never_index npm-debug.log +.vscodeignore diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index 29a50a16a36..7e9d26b9f70 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -5,19 +5,19 @@ import {module, test} from 'qunit'; import DS from 'ember-data'; -var env, store, Person; -var attr = DS.attr; -var hasMany = DS.hasMany; -var run = Ember.run; +var env, store; +const attr = DS.attr; +const hasMany = DS.hasMany; +const run = Ember.run; + +const Person = DS.Model.extend({ + firstName: attr('string'), + lastName: attr('string'), + siblings: hasMany('person') +}); module('integration/records/relationship-changes - Relationship changes', { beforeEach() { - Person = DS.Model.extend({ - firstName: attr('string'), - lastName: attr('string'), - siblings: hasMany('person') - }); - env = setupStore({ person: Person }); @@ -33,8 +33,8 @@ module('integration/records/relationship-changes - Relationship changes', { test('Calling push with relationship triggers observers once if the relationship was empty and is added to', function(assert) { assert.expect(1); - var person; - var observerCount = 0; + let person = null; + let observerCount = 0; run(function() { store.push({ @@ -92,8 +92,8 @@ test('Calling push with relationship triggers observers once if the relationship test('Calling push with relationship triggers observers once if the relationship was not empty and was added to', function(assert) { assert.expect(1); - var person; - var observerCount = 0; + let person = null; + let observerCount = 0; run(function() { store.push({ @@ -169,8 +169,8 @@ test('Calling push with relationship triggers observers once if the relationship test('Calling push with relationship triggers observers once if the relationship was made shorter', function(assert) { assert.expect(1); - var person; - var observerCount = 0; + let person = null; + let observerCount = 0; run(function() { store.push({ @@ -229,8 +229,8 @@ test('Calling push with relationship triggers observers once if the relationship test('Calling push with relationship triggers observers once if the relationship was reordered', function(assert) { assert.expect(1); - var person; - var observerCount = 0; + let person = null; + let observerCount = 0; run(function() { store.push({ @@ -298,8 +298,8 @@ test('Calling push with relationship triggers observers once if the relationship test('Calling push with relationship does not trigger observers if the relationship was not changed', function(assert) { assert.expect(1); - var person; - var observerCount = 0; + let person = null; + let observerCount = 0; run(function() { store.push({ From fd60d3e7f513c772c6c22d515d5d07358bf7f2e3 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Fri, 21 Oct 2016 18:04:49 +0100 Subject: [PATCH 03/15] add test for enumerable observer and ES6-ify --- addon/-private/system/many-array.js | 60 +++--- .../records/relationship-changes-test.js | 194 +++++++++++------- 2 files changed, 147 insertions(+), 107 deletions(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index cc7b8b4c10e..68183bc3a8c 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -6,8 +6,7 @@ import { assert } from "ember-data/-private/debug"; import { PromiseArray } from "ember-data/-private/system/promise-proxies"; import { _objectIsAlive } from "ember-data/-private/system/store/common"; -var get = Ember.get; -var set = Ember.set; +const { get, set } = Ember; /** A `ManyArray` is a `MutableArray` that represents the contents of a has-many @@ -74,27 +73,31 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { }, flushCanonical() { + // It’s possible the parent side of the relationship may have been unloaded by this point + if (!_objectIsAlive(this)) { + return; + } //TODO make this smarter, currently its plenty stupid - var toSet = this.canonicalState.filter((internalModel) => !internalModel.isDeleted()); + let toSet = this.canonicalState.filter((internalModel) => !internalModel.isDeleted()); //a hack for not removing new records //TODO remove once we have proper diffing - var newRecords = this.currentState.filter( + const newRecords = this.currentState.filter( // only add new records which are not yet in the canonical state of this // relationship (a new record can be in the canonical state if it has // been 'acknowleged' to be in the relationship via a store.push) (internalModel) => internalModel.isNew() && toSet.indexOf(internalModel) === -1 ); toSet = toSet.concat(newRecords); - var oldLength = this.length; - var newLength = toSet.length; + const oldLength = this.length; + const newLength = toSet.length; - var shortestLength = Math.min(oldLength, newLength); + const shortestLength = Math.min(oldLength, newLength); - var firstChangeIndex = -1; // -1 signifies no changes + let firstChangeIndex = -1; // -1 signifies no changes // find the first change - var currentArray = this.currentState; - for (var i=0; i 0) { - records = this.currentState.slice(idx, idx+amt); - this.get('relationship').removeRecords(records); + let records = this.currentState.slice(idx, idx+amt); + get(this, 'relationship').removeRecords(records); } if (objects) { - this.get('relationship').addRecords(objects.map((obj) => obj._internalModel), idx); + get(this, 'relationship').addRecords(objects.map((obj) => obj._internalModel), idx); } }, /** @@ -285,9 +284,9 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { @return {DS.PromiseArray} promise */ save() { - var manyArray = this; - var promiseLabel = "DS: ManyArray#save " + get(this, 'type'); - var promise = Ember.RSVP.all(this.invoke("save"), promiseLabel).then(function(array) { + const manyArray = this; + const promiseLabel = `DS: ManyArray#save ${get(this, 'type')}`; + const promise = Ember.RSVP.all(this.invoke("save"), promiseLabel).then(function(array) { return manyArray; }, null, "DS: ManyArray#save return ManyArray"); @@ -303,12 +302,11 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { @return {DS.Model} record */ createRecord(hash) { - var store = get(this, 'store'); - var type = get(this, 'type'); - var record; + const store = get(this, 'store'); + const type = get(this, 'type'); - assert("You cannot add '" + type.modelName + "' records to this polymorphic relationship.", !get(this, 'isPolymorphic')); - record = store.createRecord(type.modelName, hash); + assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !get(this, 'isPolymorphic')); + let record = store.createRecord(type.modelName, hash); this.pushObject(record); return record; diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index 7e9d26b9f70..9301cfd459e 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -5,10 +5,9 @@ import {module, test} from 'qunit'; import DS from 'ember-data'; -var env, store; -const attr = DS.attr; -const hasMany = DS.hasMany; -const run = Ember.run; +let env, store; +const { attr, hasMany } = DS; +const { run } = Ember; const Person = DS.Model.extend({ firstName: attr('string'), @@ -16,6 +15,34 @@ const Person = DS.Model.extend({ siblings: hasMany('person') }); +const sibling1 = { + type: 'person', + id: '1', + attributes: { + firstName: 'Dogzn', + lastName: 'Katz' + } +}; + +const sibling1Ref = { + type: 'person', + id: '1' +}; + +const sibling2 = { + type: 'person', + id: '2', + attributes: { + firstName: 'Katzn', + lastName: 'Dogz' + } +}; + +const sibling2Ref = { + type: 'person', + id: '2' +}; + module('integration/records/relationship-changes - Relationship changes', { beforeEach() { env = setupStore({ @@ -68,19 +95,12 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '1', type: 'person'}] + data: [sibling1Ref] } } }, included: [ - { - type: 'person', - id: '1', - attributes: { - firstName: 'Katzn', - lastName: 'Dogz' - } - } + sibling1 ] }); }); @@ -106,27 +126,12 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '1', type: 'person'}] + data: [sibling1Ref] } } }, included: [ - { - type: 'person', - id: '1', - attributes: { - firstName: 'Dogzn', - lastName: 'Katz' - } - }, - { - type: 'person', - id: '2', - attributes: { - firstName: 'Katzn', - lastName: 'Dogz' - } - } + sibling1 ] }); person = store.peekRecord('person', 'wat'); @@ -145,19 +150,12 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '1', type: 'person'}, {id: '2', type: 'person'}] + data: [sibling1Ref, sibling2Ref] } } }, included: [ - { - type: 'person', - id: '2', - attributes: { - firstName: 'Katzn', - lastName: 'Dogz' - } - } + sibling2 ] }); }); @@ -183,19 +181,12 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '1', type: 'person'}] + data: [sibling1Ref] } } }, included: [ - { - type: 'person', - id: '1', - attributes: { - firstName: 'Dogzn', - lastName: 'Katz' - } - } + sibling1 ] }); person = store.peekRecord('person', 'wat'); @@ -243,27 +234,13 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '1', type: 'person'}, {id: '2', type: 'person'}] + data: [sibling1Ref, sibling2Ref] } } }, included: [ - { - type: 'person', - id: '1', - attributes: { - firstName: 'Dogzn', - lastName: 'Katz' - } - }, - { - type: 'person', - id: '2', - attributes: { - firstName: 'Katzn', - lastName: 'Dogz' - } - } + sibling1, + sibling2 ] }); @@ -283,7 +260,7 @@ test('Calling push with relationship triggers observers once if the relationship }, relationships: { siblings: { - data: [{id: '2', type: 'person'}, {id: '1', type: 'person'}] + data: [sibling2Ref, sibling1Ref] } } }, @@ -312,19 +289,12 @@ test('Calling push with relationship does not trigger observers if the relations }, relationships: { siblings: { - data: [{id: '1', type: 'person'}] + data: [sibling1Ref] } } }, included: [ - { - type: 'person', - id: '1', - attributes: { - firstName: 'Dogzn', - lastName: 'Katz' - } - } + sibling1 ] }); @@ -344,7 +314,7 @@ test('Calling push with relationship does not trigger observers if the relations }, relationships: { siblings: { - data: [{id: '1', type: 'person'}] + data: [sibling1Ref] } } }, @@ -356,3 +326,75 @@ test('Calling push with relationship does not trigger observers if the relations assert.equal(observerCount, 0, 'siblings observer should not be triggered'); }); }); + +test('Calling push with relationship triggers willChange and didChange with detail when appending', function(assert) { + assert.expect(1); + let person = null; + let willChangeCount = 0; + let didChangeCount = 0; + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [sibling1Ref] + } + } + }, + included: [ + sibling1 + ] + + }); + person = store.peekRecord('person', 'wat'); + }); + + person.get('siblings') + .then(siblings => { + siblings.addEnumerableObserver(this, { + willChange: function(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 0); + assert.equal(adding, 1); + }, + didChange:function(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 0); + assert.equal(added, 1); + } + }); + }); + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling2Ref] + } + } + }, + included: [ + sibling2 + ] + }); + }); + + run(function() { + assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); + assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + }); +}); From 262528282accda6151876ad64963cdf8d84b09f5 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Sat, 22 Oct 2016 17:36:54 +0100 Subject: [PATCH 04/15] implement subset array diff in has-many to optimise notifications --- addon/-private/system/many-array.js | 15 +- .../records/relationship-changes-test.js | 371 +++++++++++++++++- 2 files changed, 368 insertions(+), 18 deletions(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 68183bc3a8c..28a195ae04b 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -111,9 +111,18 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { } } if (firstChangeIndex !== -1) { - // we found a change - const added = newLength - firstChangeIndex; - const removed = oldLength - firstChangeIndex; + // we found a change, find the end of the change + let unchangedEndBlockLength = 0; + // walk back from the end of both arrays until we find a change + for (let i=1; i { + siblings.addArrayObserver(this, { + arrayWillChange(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 0); + assert.equal(adding, 1); + }, + arrayDidChange(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 0); + assert.equal(added, 1); + } + }); + }); }); - person.get('siblings') - .then(siblings => { - siblings.addEnumerableObserver(this, { - willChange: function(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 1); - assert.equal(removing, 0); - assert.equal(adding, 1); + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling2Ref] + } + } }, - didChange:function(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 1); - assert.equal(removed, 0); - assert.equal(added, 1); - } + included: [ + sibling2 + ] }); }); + run(function() { + assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); + assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + done(); + }); +}); + +test('Calling push with relationship triggers willChange and didChange with detail when truncating', function(assert) { + const done = assert.async(); + let person = null; + let willChangeCount = 0; + let didChangeCount = 0; + run(function() { store.push({ data: { type: 'person', id: 'wat', attributes: { + firstName: 'Yehuda', + lastName: 'Katz' }, relationships: { siblings: { @@ -387,14 +463,279 @@ test('Calling push with relationship triggers willChange and didChange with deta } } }, + included: [ + sibling1, sibling2 + ] + + }); + person = store.peekRecord('person', 'wat'); + + person.get('siblings') + .then(siblings => { + siblings.addArrayObserver(this, { + arrayWillChange(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 1); + assert.equal(adding, 0); + }, + arrayDidChange(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 1); + assert.equal(added, 0); + } + }); + }); + + }); + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref] + } + } + }, + included: [] + }); + }); + + run(function() { + assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); + assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + done(); + }); +}); + +test('Calling push with relationship triggers willChange and didChange with detail when inserting at front', function(assert) { + const done = assert.async(); + let person = null; + let willChangeCount = 0; + let didChangeCount = 0; + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [sibling2Ref] + } + } + }, included: [ sibling2 ] + + }); + person = store.peekRecord('person', 'wat'); + + person.get('siblings') + .then(siblings => { + siblings.addArrayObserver(this, { + arrayWillChange(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 0); + assert.equal(removing, 0); + assert.equal(adding, 1); + }, + arrayDidChange(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 0); + assert.equal(removed, 0); + assert.equal(added, 1); + } + }); + }); + + }); + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling2Ref] + } + } + }, + included: [ + sibling2 + ] + }); + }); + + run(function() { + assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); + assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + done(); + }); +}); + +test('Calling push with relationship triggers willChange and didChange with detail when inserting in middle', function(assert) { + const done = assert.async(); + let person = null; + let willChangeCount = 0; + let didChangeCount = 0; + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling3Ref] + } + } + }, + included: [ + sibling1, + sibling3 + ] + + }); + person = store.peekRecord('person', 'wat'); + + person.get('siblings') + .then(siblings => { + siblings.addArrayObserver(this, { + arrayWillChange(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 0); + assert.equal(adding, 1); + }, + arrayDidChange(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 0); + assert.equal(added, 1); + } + }); + }); + + }); + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling2Ref, sibling3Ref] + } + } + }, + included: [ + sibling2 + ] + }); + }); + + run(function() { + assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); + assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + done(); + }); +}); + +test('Calling push with relationship triggers willChange and didChange with detail when replacing different length in middle', function(assert) { + const done = assert.async(); + let person = null; + let willChangeCount = 0; + let didChangeCount = 0; + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling2Ref, sibling3Ref] + } + } + }, + included: [ + sibling1, + sibling2, + sibling3 + ] + + }); + person = store.peekRecord('person', 'wat'); + + person.get('siblings') + .then(siblings => { + siblings.addArrayObserver(this, { + arrayWillChange(array, start, removing, adding) { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 1); + assert.equal(adding, 2); + }, + arrayDidChange(array, start, removed, added) { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 1); + assert.equal(added, 2); + } + }); + }); + + }); + + run(function() { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref, sibling4Ref, sibling5Ref, sibling3Ref] + } + } + }, + included: [ + sibling4, + sibling5 + ] }); }); run(function() { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); + done(); }); }); From 3674dcad0f31db3de0e5cfcd2bcfea5e596a4759 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Thu, 15 Dec 2016 03:40:46 +0000 Subject: [PATCH 05/15] correct order of params in array notifications --- addon/-private/system/many-array.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 28a195ae04b..0da51b937a8 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -123,10 +123,10 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { } const added = newLength - unchangedEndBlockLength - firstChangeIndex; const removed = oldLength - unchangedEndBlockLength - firstChangeIndex; - this.arrayContentWillChange(firstChangeIndex, added, removed); + this.arrayContentWillChange(firstChangeIndex, removed, added); set(this, 'length', toSet.length); this.currentState = toSet; - this.arrayContentDidChange(firstChangeIndex, added, removed); + this.arrayContentDidChange(firstChangeIndex, removed, added); this.relationship.notifyHasManyChanged(); } this.record.updateRecordArrays(); From 6b71d772e3bf13f139cf7a4d9783e69f5707f9d4 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 11:08:01 +0000 Subject: [PATCH 06/15] refactor diff-array out as function for ease of testing --- addon/-private/system/diff-array.js | 58 ++++ addon/-private/system/many-array.js | 46 +-- jsconfig.json | 1 + tests/unit/diff-array-test.js | 486 ++++++++++++++++++++++++++++ 4 files changed, 555 insertions(+), 36 deletions(-) create mode 100644 addon/-private/system/diff-array.js create mode 100644 jsconfig.json create mode 100644 tests/unit/diff-array-test.js diff --git a/addon/-private/system/diff-array.js b/addon/-private/system/diff-array.js new file mode 100644 index 00000000000..aadafc77b6a --- /dev/null +++ b/addon/-private/system/diff-array.js @@ -0,0 +1,58 @@ +/** + @namespace + @method diff-array + @for DS + @param {Array} oldArray the old array + @param {Array} newArray the new array + @return {hash} { + firstChangeIndex: , // null if no change + addedCount: , // 0 if no change + removedCount: // 0 if no change + } +*/ +export default function (oldArray, newArray) { + const oldLength = oldArray.length; + const newLength = newArray.length; + + const shortestLength = Math.min(oldLength, newLength); + let firstChangeIndex = null; // null signifies no changes + + // find the first change + for (let i=0; i internalModel.isNew() && toSet.indexOf(internalModel) === -1 ); toSet = toSet.concat(newRecords); - const oldLength = this.length; - const newLength = toSet.length; - - const shortestLength = Math.min(oldLength, newLength); - - let firstChangeIndex = -1; // -1 signifies no changes - // find the first change - const currentArray = this.currentState; - for (let i=0; i Date: Wed, 8 Mar 2017 11:16:50 +0000 Subject: [PATCH 07/15] fix most tests --- addon/-private/system/many-array.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 0aacd6b4559..42bf5a3b6b4 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -171,7 +171,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { // diff to find changes const diff = diffArray(this.currentState, toSet); - if (diff.firstChangeIndex) { // it's null if no change found + if (diff.firstChangeIndex !== null) { // it's null if no change found // we found a change this.arrayContentWillChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount); set(this, 'length', toSet.length); From 27e2f11a1d30ba94cc9eb3b46a47b50cc4603529 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 13:19:26 +0000 Subject: [PATCH 08/15] fix array observer tests - not sure how these tests used to pass --- .../records/relationship-changes-test.js | 151 ++++++++++-------- 1 file changed, 81 insertions(+), 70 deletions(-) diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index 7ce0b6346d8..c7668696c1d 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -385,6 +385,8 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; + let test = this; + run(function() { store.push({ data: { @@ -407,22 +409,22 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); + test.arrayWillChange = (array, start, removing, adding) => { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 0); + assert.equal(adding, 1); + }; + test.arrayDidChange = (array, start, removed, added) => { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 0); + assert.equal(added, 1); + }; + person.get('siblings') .then(siblings => { - siblings.addArrayObserver(this, { - arrayWillChange(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 1); - assert.equal(removing, 0); - assert.equal(adding, 1); - }, - arrayDidChange(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 1); - assert.equal(removed, 0); - assert.equal(added, 1); - } - }); + siblings.addArrayObserver(test); }); }); @@ -458,6 +460,8 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; + let test = this; + run(function() { store.push({ data: { @@ -480,22 +484,22 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); + test.arrayWillChange = (array, start, removing, adding) => { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 1); + assert.equal(adding, 0); + }; + test.arrayDidChange = (array, start, removed, added) => { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 1); + assert.equal(added, 0); + }; + person.get('siblings') .then(siblings => { - siblings.addArrayObserver(this, { - arrayWillChange(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 1); - assert.equal(removing, 1); - assert.equal(adding, 0); - }, - arrayDidChange(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 1); - assert.equal(removed, 1); - assert.equal(added, 0); - } - }); + siblings.addArrayObserver(test); }); }); @@ -530,6 +534,8 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; + let test = this; + run(function() { store.push({ data: { @@ -552,22 +558,22 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); + test.arrayWillChange = (array, start, removing, adding) => { + willChangeCount++; + assert.equal(start, 0); + assert.equal(removing, 0); + assert.equal(adding, 1); + }; + test.arrayDidChange = (array, start, removed, added) => { + didChangeCount++; + assert.equal(start, 0); + assert.equal(removed, 0); + assert.equal(added, 1); + }; + person.get('siblings') .then(siblings => { - siblings.addArrayObserver(this, { - arrayWillChange(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 0); - assert.equal(removing, 0); - assert.equal(adding, 1); - }, - arrayDidChange(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 0); - assert.equal(removed, 0); - assert.equal(added, 1); - } - }); + siblings.addArrayObserver(test); }); }); @@ -604,6 +610,8 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; + let test = this; + run(function() { store.push({ data: { @@ -627,22 +635,22 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); + test.arrayWillChange = (array, start, removing, adding) => { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 0); + assert.equal(adding, 1); + }; + test.arrayDidChange = (array, start, removed, added) => { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 0); + assert.equal(added, 1); + }; + person.get('siblings') .then(siblings => { - siblings.addArrayObserver(this, { - arrayWillChange(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 1); - assert.equal(removing, 0); - assert.equal(adding, 1); - }, - arrayDidChange(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 1); - assert.equal(removed, 0); - assert.equal(added, 1); - } - }); + siblings.addArrayObserver(test); }); }); @@ -679,6 +687,8 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; + let test = this; + run(function() { store.push({ data: { @@ -703,22 +713,23 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); + test.arrayWillChange = (array, start, removing, adding) => { + willChangeCount++; + assert.equal(start, 1); + assert.equal(removing, 1); + assert.equal(adding, 2); + }; + + test.arrayDidChange = (array, start, removed, added) => { + didChangeCount++; + assert.equal(start, 1); + assert.equal(removed, 1); + assert.equal(added, 2); + }; + person.get('siblings') .then(siblings => { - siblings.addArrayObserver(this, { - arrayWillChange(array, start, removing, adding) { - willChangeCount++; - assert.equal(start, 1); - assert.equal(removing, 1); - assert.equal(adding, 2); - }, - arrayDidChange(array, start, removed, added) { - didChangeCount++; - assert.equal(start, 1); - assert.equal(removed, 1); - assert.equal(added, 2); - } - }); + siblings.addArrayObserver(test); }); }); From b199170ec229e3a3196085eafc4831824203ebd1 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 13:20:14 +0000 Subject: [PATCH 09/15] observe the many arrays themselves in tests - should surely be able to observe siblings.[]? --- .../records/relationship-changes-test.js | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index c7668696c1d..e8004313c24 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -134,8 +134,10 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - person.addObserver('siblings', function() { - observerCount++; + run(function() { + person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + observerCount++; + }); }); run(function() { @@ -189,8 +191,10 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - person.addObserver('siblings', function() { - observerCount++; + run(function() { + person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + observerCount++; + }); }); run(function() { @@ -244,10 +248,13 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - person.addObserver('siblings', function() { - observerCount++; + run(function() { + person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + observerCount++; + }); }); + run(function() { store.push({ data: { @@ -299,10 +306,13 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - person.addObserver('siblings', function() { - observerCount++; + run(function() { + person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + observerCount++; + }); }); + run(function() { store.push({ data: { @@ -353,10 +363,13 @@ test('Calling push with relationship does not trigger observers if the relations person = store.peekRecord('person', 'wat'); }); - person.addObserver('siblings', function() { - observerCount++; + run(function() { + person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + observerCount++; + }); }); + run(function() { store.push({ data: { From 8fe3411c127877d1b00db30101707ef37e733229 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 19:13:22 +0000 Subject: [PATCH 10/15] address feedback and bring many-array into line with master --- addon/-private/system/many-array.js | 56 ++++----- .../records/relationship-changes-test.js | 116 ++++++++---------- 2 files changed, 77 insertions(+), 95 deletions(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 42bf5a3b6b4..79980c2e77c 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -5,7 +5,6 @@ import Ember from 'ember'; import { assert } from "ember-data/-private/debug"; import { PromiseArray } from "ember-data/-private/system/promise-proxies"; import { _objectIsAlive } from "ember-data/-private/system/store/common"; - import diffArray from './diff-array'; const { get, set } = Ember; @@ -54,16 +53,9 @@ const { get, set } = Ember; @uses Ember.MutableArray, Ember.Evented */ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { - record: null, - - canonicalState: null, - currentState: null, - - length: 0, - init() { this._super(...arguments); - this.currentState = Ember.A([]); + /** The loading state of this array @@ -150,13 +142,8 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { return this.currentState[index].getRecord(); }, - flushCanonical() { - // It’s possible the parent side of the relationship may have been unloaded by this point - if (!_objectIsAlive(this)) { - return; - } - //TODO make this smarter, currently its plenty stupid - let toSet = this.canonicalState.filter((internalModel) => !internalModel.isDeleted()); + flushCanonical(isInitialized = true) { + let toSet = this.canonicalState; //a hack for not removing new records //TODO remove once we have proper diffing @@ -173,13 +160,18 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { if (diff.firstChangeIndex !== null) { // it's null if no change found // we found a change - this.arrayContentWillChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount); - set(this, 'length', toSet.length); + this.arrayContentWillChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount); + // It’s possible the parent side of the relationship may have been unloaded by this point + if (_objectIsAlive(this)) { + this.set('length', toSet.length); + } this.currentState = toSet; - this.arrayContentDidChange(diff.firstChangeIndex || 0, diff.removedCount, diff.addedCount); + this.arrayContentDidChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount); this.relationship.notifyHasManyChanged(); } - this.record.updateRecordArrays(); + if (isInitialized) { + this.record.updateRecordArrays(); + } }, internalReplace(idx, amt, objects) { @@ -188,15 +180,14 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { } this.arrayContentWillChange(idx, amt, objects.length); this.currentState.splice.apply(this.currentState, [idx, amt].concat(objects)); - set(this, 'length', this.currentState.length); + this.set('length', this.currentState.length); this.arrayContentDidChange(idx, amt, objects.length); }, //TODO(Igor) optimize internalRemoveRecords(records) { - let index; for (let i=0; i < records.length; i++) { - index = this.currentState.indexOf(records[i]); + let index = this.currentState.indexOf(records[i]); this.internalReplace(index, 1); } }, @@ -210,12 +201,13 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { }, replace(idx, amt, objects) { + let records; if (amt > 0) { - const records = this.currentState.slice(idx, idx+amt); - get(this, 'relationship').removeRecords(records); + records = this.currentState.slice(idx, idx+amt); + this.get('relationship').removeRecords(records); } if (objects) { - get(this, 'relationship').addRecords(objects.map(obj => obj._internalModel), idx); + this.get('relationship').addRecords(objects.map(obj => obj._internalModel), idx); } }, @@ -287,10 +279,10 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { @return {DS.PromiseArray} promise */ save() { - const manyArray = this; - const promiseLabel = `DS: ManyArray#save ${get(this, 'type')}`; - const promise = Ember.RSVP.all(this.invoke("save"), promiseLabel) - .then(() => manyArray, null, "DS: ManyArray#save return ManyArray"); + let manyArray = this; + let promiseLabel = 'DS: ManyArray#save ' + get(this, 'type'); + let promise = Ember.RSVP.all(this.invoke("save"), promiseLabel). + then(() => manyArray, null, 'DS: ManyArray#save return ManyArray'); return PromiseArray.create({ promise }); }, @@ -304,10 +296,10 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { @return {DS.Model} record */ createRecord(hash) { + const store = get(this, 'store'); const type = get(this, 'type'); - assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !get(this, 'isPolymorphic')); - const store = get(this, 'store'); + assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !get(this, 'isPolymorphic')); const record = store.createRecord(type.modelName, hash); this.pushObject(record); diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index e8004313c24..fe12acf4cdb 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -104,7 +104,7 @@ module('integration/records/relationship-changes - Relationship changes', { }, afterEach() { - run(function() { + run(() => { env.container.destroy(); }); } @@ -115,7 +115,7 @@ test('Calling push with relationship triggers observers once if the relationship let person = null; let observerCount = 0; - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -134,13 +134,13 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - run(function() { + run(() => { person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { observerCount++; }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -159,7 +159,7 @@ test('Calling push with relationship triggers observers once if the relationship }); }); - run(function() { + run(() => { assert.equal(observerCount, 1, 'siblings observer should be triggered once'); }); }); @@ -169,7 +169,7 @@ test('Calling push with relationship triggers observers once if the relationship let person = null; let observerCount = 0; - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -191,13 +191,13 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - run(function() { + run(() => { person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { observerCount++; }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -216,7 +216,7 @@ test('Calling push with relationship triggers observers once if the relationship }); }); - run(function() { + run(() => { assert.equal(observerCount, 1, 'siblings observer should be triggered once'); }); }); @@ -226,7 +226,7 @@ test('Calling push with relationship triggers observers once if the relationship let person = null; let observerCount = 0; - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -248,14 +248,14 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - run(function() { + run(() => { person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { observerCount++; }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -272,7 +272,7 @@ test('Calling push with relationship triggers observers once if the relationship }); }); - run(function() { + run(() => { assert.equal(observerCount, 1, 'siblings observer should be triggered once'); }); }); @@ -282,7 +282,7 @@ test('Calling push with relationship triggers observers once if the relationship let person = null; let observerCount = 0; - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -306,14 +306,14 @@ test('Calling push with relationship triggers observers once if the relationship person = store.peekRecord('person', 'wat'); }); - run(function() { + run(() => { person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { observerCount++; }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -330,7 +330,7 @@ test('Calling push with relationship triggers observers once if the relationship }); }); - run(function() { + run(() => { assert.equal(observerCount, 1, 'siblings observer should be triggered once'); }); }); @@ -340,7 +340,7 @@ test('Calling push with relationship does not trigger observers if the relations let person = null; let observerCount = 0; - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -363,14 +363,14 @@ test('Calling push with relationship does not trigger observers if the relations person = store.peekRecord('person', 'wat'); }); - run(function() { + run(() => { person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { observerCount++; }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -387,7 +387,7 @@ test('Calling push with relationship does not trigger observers if the relations }); }); - run(function() { + run(() => { assert.equal(observerCount, 0, 'siblings observer should not be triggered'); }); }); @@ -398,9 +398,7 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; - let test = this; - - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -422,13 +420,13 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); - test.arrayWillChange = (array, start, removing, adding) => { + this.arrayWillChange = (array, start, removing, adding) => { willChangeCount++; assert.equal(start, 1); assert.equal(removing, 0); assert.equal(adding, 1); }; - test.arrayDidChange = (array, start, removed, added) => { + this.arrayDidChange = (array, start, removed, added) => { didChangeCount++; assert.equal(start, 1); assert.equal(removed, 0); @@ -437,11 +435,11 @@ test('Calling push with relationship triggers willChange and didChange with deta person.get('siblings') .then(siblings => { - siblings.addArrayObserver(test); + siblings.addArrayObserver(this); }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -460,7 +458,7 @@ test('Calling push with relationship triggers willChange and didChange with deta }); }); - run(function() { + run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); done(); @@ -473,9 +471,7 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; - let test = this; - - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -497,13 +493,13 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); - test.arrayWillChange = (array, start, removing, adding) => { + this.arrayWillChange = (array, start, removing, adding) => { willChangeCount++; assert.equal(start, 1); assert.equal(removing, 1); assert.equal(adding, 0); }; - test.arrayDidChange = (array, start, removed, added) => { + this.arrayDidChange = (array, start, removed, added) => { didChangeCount++; assert.equal(start, 1); assert.equal(removed, 1); @@ -512,12 +508,12 @@ test('Calling push with relationship triggers willChange and didChange with deta person.get('siblings') .then(siblings => { - siblings.addArrayObserver(test); + siblings.addArrayObserver(this); }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -534,7 +530,7 @@ test('Calling push with relationship triggers willChange and didChange with deta }); }); - run(function() { + run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); done(); @@ -547,9 +543,7 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; - let test = this; - - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -571,13 +565,13 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); - test.arrayWillChange = (array, start, removing, adding) => { + this.arrayWillChange = (array, start, removing, adding) => { willChangeCount++; assert.equal(start, 0); assert.equal(removing, 0); assert.equal(adding, 1); }; - test.arrayDidChange = (array, start, removed, added) => { + this.arrayDidChange = (array, start, removed, added) => { didChangeCount++; assert.equal(start, 0); assert.equal(removed, 0); @@ -586,12 +580,12 @@ test('Calling push with relationship triggers willChange and didChange with deta person.get('siblings') .then(siblings => { - siblings.addArrayObserver(test); + siblings.addArrayObserver(this); }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -610,7 +604,7 @@ test('Calling push with relationship triggers willChange and didChange with deta }); }); - run(function() { + run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); done(); @@ -623,9 +617,7 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; - let test = this; - - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -648,13 +640,13 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); - test.arrayWillChange = (array, start, removing, adding) => { + this.arrayWillChange = (array, start, removing, adding) => { willChangeCount++; assert.equal(start, 1); assert.equal(removing, 0); assert.equal(adding, 1); }; - test.arrayDidChange = (array, start, removed, added) => { + this.arrayDidChange = (array, start, removed, added) => { didChangeCount++; assert.equal(start, 1); assert.equal(removed, 0); @@ -663,12 +655,12 @@ test('Calling push with relationship triggers willChange and didChange with deta person.get('siblings') .then(siblings => { - siblings.addArrayObserver(test); + siblings.addArrayObserver(this); }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -687,7 +679,7 @@ test('Calling push with relationship triggers willChange and didChange with deta }); }); - run(function() { + run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); done(); @@ -700,9 +692,7 @@ test('Calling push with relationship triggers willChange and didChange with deta let willChangeCount = 0; let didChangeCount = 0; - let test = this; - - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -726,14 +716,14 @@ test('Calling push with relationship triggers willChange and didChange with deta }); person = store.peekRecord('person', 'wat'); - test.arrayWillChange = (array, start, removing, adding) => { + this.arrayWillChange = (array, start, removing, adding) => { willChangeCount++; assert.equal(start, 1); assert.equal(removing, 1); assert.equal(adding, 2); }; - test.arrayDidChange = (array, start, removed, added) => { + this.arrayDidChange = (array, start, removed, added) => { didChangeCount++; assert.equal(start, 1); assert.equal(removed, 1); @@ -742,12 +732,12 @@ test('Calling push with relationship triggers willChange and didChange with deta person.get('siblings') .then(siblings => { - siblings.addArrayObserver(test); + siblings.addArrayObserver(this); }); }); - run(function() { + run(() => { store.push({ data: { type: 'person', @@ -767,7 +757,7 @@ test('Calling push with relationship triggers willChange and didChange with deta }); }); - run(function() { + run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); done(); @@ -779,7 +769,7 @@ test('Calling push with updated belongsTo relationship trigger observer', functi let observerCount = 0; - run(function() { + run(() => { let post = env.store.push({ data: { type: 'post', @@ -817,7 +807,7 @@ test('Calling push with same belongsTo relationship does not trigger observer', let observerCount = 0; - run(function() { + run(() => { let post = env.store.push({ data: { type: 'post', From 993dd607d970479cfc5f3ec55b8c18637021e1d9 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 19:17:00 +0000 Subject: [PATCH 11/15] apply feedback --- .gitignore | 1 - addon/-private/system/diff-array.js | 4 +++- tests/integration/records/relationship-changes-test.js | 10 ---------- tests/unit/diff-array-test.js | 5 ----- 4 files changed, 3 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index 8baa706910a..d7b48a542b4 100644 --- a/.gitignore +++ b/.gitignore @@ -23,4 +23,3 @@ node_modules/ bower_components/ .metadata_never_index npm-debug.log -.vscodeignore diff --git a/addon/-private/system/diff-array.js b/addon/-private/system/diff-array.js index aadafc77b6a..636541a4f89 100644 --- a/addon/-private/system/diff-array.js +++ b/addon/-private/system/diff-array.js @@ -10,7 +10,7 @@ removedCount: // 0 if no change } */ -export default function (oldArray, newArray) { +const diffArray = function (oldArray, newArray) { const oldLength = oldArray.length; const newLength = newArray.length; @@ -56,3 +56,5 @@ export default function (oldArray, newArray) { removedCount }; } + +export default diffArray; diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index fe12acf4cdb..9bcf152f4c3 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -393,7 +393,6 @@ test('Calling push with relationship does not trigger observers if the relations }); test('Calling push with relationship triggers willChange and didChange with detail when appending', function(assert) { - const done = assert.async(); let person = null; let willChangeCount = 0; let didChangeCount = 0; @@ -461,12 +460,10 @@ test('Calling push with relationship triggers willChange and didChange with deta run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); - done(); }); }); test('Calling push with relationship triggers willChange and didChange with detail when truncating', function(assert) { - const done = assert.async(); let person = null; let willChangeCount = 0; let didChangeCount = 0; @@ -533,12 +530,10 @@ test('Calling push with relationship triggers willChange and didChange with deta run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); - done(); }); }); test('Calling push with relationship triggers willChange and didChange with detail when inserting at front', function(assert) { - const done = assert.async(); let person = null; let willChangeCount = 0; let didChangeCount = 0; @@ -607,12 +602,10 @@ test('Calling push with relationship triggers willChange and didChange with deta run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); - done(); }); }); test('Calling push with relationship triggers willChange and didChange with detail when inserting in middle', function(assert) { - const done = assert.async(); let person = null; let willChangeCount = 0; let didChangeCount = 0; @@ -682,12 +675,10 @@ test('Calling push with relationship triggers willChange and didChange with deta run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); - done(); }); }); test('Calling push with relationship triggers willChange and didChange with detail when replacing different length in middle', function(assert) { - const done = assert.async(); let person = null; let willChangeCount = 0; let didChangeCount = 0; @@ -760,7 +751,6 @@ test('Calling push with relationship triggers willChange and didChange with deta run(() => { assert.equal(willChangeCount, 1, 'willChange observer should be triggered once'); assert.equal(didChangeCount, 1, 'didChange observer should be triggered once'); - done(); }); }); diff --git a/tests/unit/diff-array-test.js b/tests/unit/diff-array-test.js index 2815bf85611..a39ffa26073 100644 --- a/tests/unit/diff-array-test.js +++ b/tests/unit/diff-array-test.js @@ -3,11 +3,6 @@ import {module, test} from 'qunit'; import diffArray from 'ember-data/-private/system/diff-array'; module('unit/diff-array Diff Array tests', { - beforeEach() { - }, - - afterEach() { - } }); const a = "aaa"; From df30a5f2e17aaceb096035e26f4db49d5c433ac2 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 19:18:14 +0000 Subject: [PATCH 12/15] remove jsconfig.json --- jsconfig.json | 1 - 1 file changed, 1 deletion(-) delete mode 100644 jsconfig.json diff --git a/jsconfig.json b/jsconfig.json deleted file mode 100644 index f408cac8c29..00000000000 --- a/jsconfig.json +++ /dev/null @@ -1 +0,0 @@ -{"compilerOptions":{"target":"es6","experimentalDecorators":true},"exclude":["node_modules","bower_components","tmp","vendor",".git","dist"]} \ No newline at end of file From 6e67c3d67440d0fc364aad6753b7f977f950303b Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Wed, 8 Mar 2017 21:45:52 +0000 Subject: [PATCH 13/15] bring in line with master --- addon/-private/system/many-array.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addon/-private/system/many-array.js b/addon/-private/system/many-array.js index 79980c2e77c..1c7e5cb81a2 100644 --- a/addon/-private/system/many-array.js +++ b/addon/-private/system/many-array.js @@ -3,8 +3,8 @@ */ import Ember from 'ember'; import { assert } from "ember-data/-private/debug"; -import { PromiseArray } from "ember-data/-private/system/promise-proxies"; -import { _objectIsAlive } from "ember-data/-private/system/store/common"; +import { PromiseArray } from "./promise-proxies"; +import { _objectIsAlive } from "./store/common"; import diffArray from './diff-array'; const { get, set } = Ember; From e8c2c86cc41bc7fed96b1024270d9c7d8b442aea Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Thu, 9 Mar 2017 01:26:42 +0000 Subject: [PATCH 14/15] add tests for CP hooked to many-array --- .../records/relationship-changes-test.js | 167 +++++++++++++++++- .../integration/{ => store}/peek-all-test.js | 0 2 files changed, 166 insertions(+), 1 deletion(-) rename tests/integration/{ => store}/peek-all-test.js (100%) diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index 9bcf152f4c3..b2595315735 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -4,7 +4,7 @@ import Ember from 'ember'; import DS from 'ember-data'; import { module, test } from 'qunit'; -const { run } = Ember; +const { run, get, set, computed } = Ember; const { attr, belongsTo, hasMany, Model } = DS; let env, store; @@ -164,6 +164,171 @@ test('Calling push with relationship triggers observers once if the relationship }); }); +test('Calling push with relationship recalculates computed alias property if the relationship was empty and is added to', function(assert) { + assert.expect(1); + + let Obj = Ember.Object.extend({ + person: null, + siblings: computed.alias('person.siblings') + }); + + const obj = Obj.create(); + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [] + } + } + } + }); + set(obj, 'person', store.peekRecord('person', 'wat')); + }); + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref] + } + } + }, + included: [ + sibling1 + ] + }); + }); + + run(() => { + let cpResult = get(obj, 'siblings').toArray(); + assert.equal(cpResult.length, 1, 'siblings cp should have recalculated'); + obj.destroy(); + }); +}); + +test('Calling push with relationship recalculates computed alias property to firstObject if the relationship was empty and is added to', function(assert) { + assert.expect(1); + + let Obj = Ember.Object.extend({ + person: null, + firstSibling: computed.alias('person.siblings.firstObject') + }); + + const obj = Obj.create(); + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [] + } + } + } + }); + set(obj, 'person', store.peekRecord('person', 'wat')); + }); + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref] + } + } + }, + included: [ + sibling1 + ] + }); + }); + + run(() => { + let cpResult = get(obj, 'firstSibling'); + assert.equal(get(cpResult, 'id'), 1, 'siblings cp should have recalculated'); + obj.destroy(); + }); +}); + +test('Calling push with relationship triggers observer of array if the relationship was empty and is added to', function(assert) { + assert.expect(1); + let person = null; + let observerCount = 0; + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + firstName: 'Yehuda', + lastName: 'Katz' + }, + relationships: { + siblings: { + data: [] + } + } + } + }); + }); + + person = store.peekRecord('person', 'wat'); + + run(() => { + person.addObserver('siblings.[]', function() { + observerCount++; + }); + }); + + run(() => { + store.push({ + data: { + type: 'person', + id: 'wat', + attributes: { + }, + relationships: { + siblings: { + data: [sibling1Ref] + } + } + }, + included: [ + sibling1 + ] + }); + }); + + run(() => { + assert.equal(observerCount, 1, 'siblings observer should be triggered once'); + }); +}); + test('Calling push with relationship triggers observers once if the relationship was not empty and was added to', function(assert) { assert.expect(1); let person = null; diff --git a/tests/integration/peek-all-test.js b/tests/integration/store/peek-all-test.js similarity index 100% rename from tests/integration/peek-all-test.js rename to tests/integration/store/peek-all-test.js From 90dbc65a4e283139f08b806d8aec233c152c9d32 Mon Sep 17 00:00:00 2001 From: Bryan Crotaz Date: Thu, 9 Mar 2017 11:04:30 +0000 Subject: [PATCH 15/15] fix observer tests --- .../records/relationship-changes-test.js | 83 +++++-------------- 1 file changed, 19 insertions(+), 64 deletions(-) diff --git a/tests/integration/records/relationship-changes-test.js b/tests/integration/records/relationship-changes-test.js index b2595315735..54fc3486573 100644 --- a/tests/integration/records/relationship-changes-test.js +++ b/tests/integration/records/relationship-changes-test.js @@ -135,9 +135,11 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + person.addObserver('siblings.[]', function() { observerCount++; }); + // prime the pump + person.get('siblings'); }); run(() => { @@ -160,7 +162,7 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - assert.equal(observerCount, 1, 'siblings observer should be triggered once'); + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); }); }); @@ -274,61 +276,6 @@ test('Calling push with relationship recalculates computed alias property to fir }); }); -test('Calling push with relationship triggers observer of array if the relationship was empty and is added to', function(assert) { - assert.expect(1); - let person = null; - let observerCount = 0; - - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: { - firstName: 'Yehuda', - lastName: 'Katz' - }, - relationships: { - siblings: { - data: [] - } - } - } - }); - }); - - person = store.peekRecord('person', 'wat'); - - run(() => { - person.addObserver('siblings.[]', function() { - observerCount++; - }); - }); - - run(() => { - store.push({ - data: { - type: 'person', - id: 'wat', - attributes: { - }, - relationships: { - siblings: { - data: [sibling1Ref] - } - } - }, - included: [ - sibling1 - ] - }); - }); - - run(() => { - assert.equal(observerCount, 1, 'siblings observer should be triggered once'); - }); -}); - test('Calling push with relationship triggers observers once if the relationship was not empty and was added to', function(assert) { assert.expect(1); let person = null; @@ -357,9 +304,11 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + person.addObserver('siblings.[]', function() { observerCount++; }); + // prime the pump + person.get('siblings'); }); run(() => { @@ -382,7 +331,7 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - assert.equal(observerCount, 1, 'siblings observer should be triggered once'); + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); }); }); @@ -414,9 +363,11 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + person.addObserver('siblings.[]', function() { observerCount++; }); + // prime the pump + person.get('siblings'); }); @@ -438,7 +389,7 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - assert.equal(observerCount, 1, 'siblings observer should be triggered once'); + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); }); }); @@ -472,9 +423,11 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + person.addObserver('siblings.[]', function() { observerCount++; }); + // prime the pump + person.get('siblings'); }); @@ -496,7 +449,7 @@ test('Calling push with relationship triggers observers once if the relationship }); run(() => { - assert.equal(observerCount, 1, 'siblings observer should be triggered once'); + assert.ok(observerCount >= 1, 'siblings observer should be triggered at least once'); }); }); @@ -529,7 +482,9 @@ test('Calling push with relationship does not trigger observers if the relations }); run(() => { - person.hasMany('siblings').hasManyRelationship.getManyArray().addObserver('[]', function() { + // prime the pump + person.get('siblings'); + person.addObserver('siblings.[]', function() { observerCount++; }); });