From 7821e9e37f81b6dc8b002cc17670aa98ad441b44 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Fri, 21 Oct 2016 17:52:20 +0300 Subject: [PATCH] tidy up proxy related change events for AdapterPopulatedRecordArray during loadRecords Replace the values contained within content, rather then replacing content. Without this we: * signal `content` changed (no point) * signal strange array mutations on the ArrayProxy (from empty content, instead of previous content); Backfill missing tests --- .../adapter-populated-record-array.js | 15 +- .../adapter-populated-record-array-test.js | 103 ------------- .../adapter-populated-record-array-test.js | 142 ++++++++++++++++-- 3 files changed, 143 insertions(+), 117 deletions(-) diff --git a/addon/-private/system/record-arrays/adapter-populated-record-array.js b/addon/-private/system/record-arrays/adapter-populated-record-array.js index 5c73727d1a1..cd18d9cc1aa 100644 --- a/addon/-private/system/record-arrays/adapter-populated-record-array.js +++ b/addon/-private/system/record-arrays/adapter-populated-record-array.js @@ -47,6 +47,9 @@ const { get } = Ember; */ export default RecordArray.extend({ init() { + // yes we are touching `this` before super, but ArrayProxy has a bug that requires this. + this.set('content', this.get('content') || Ember.A()); + this._super(...arguments); this.query = this.query || null; this.links = null; @@ -73,15 +76,19 @@ export default RecordArray.extend({ */ loadRecords(internalModels, payload) { let token = heimdall.start('AdapterPopulatedRecordArray.loadRecords'); + + // TODO: initial load should not cause change events at all, only + // subsequent. This requires changing the public api of adapter.query, but + // hopefully we can do that soon. + this.get('content').setObjects(internalModels); + this.setProperties({ - content: Ember.A(internalModels), isLoaded: true, isUpdating: false, - meta: cloneNull(payload.meta) + meta: cloneNull(payload.meta), + links: cloneNull(payload.links) }); - this.set('links', cloneNull(payload.links)); - internalModels.forEach(record => { this.manager.recordArraysForRecord(record).add(this); }); diff --git a/tests/integration/record-arrays/adapter-populated-record-array-test.js b/tests/integration/record-arrays/adapter-populated-record-array-test.js index d6576079d3d..7fc4ec5c406 100644 --- a/tests/integration/record-arrays/adapter-populated-record-array-test.js +++ b/tests/integration/record-arrays/adapter-populated-record-array-test.js @@ -217,106 +217,3 @@ test('when an adapter populated record gets updated the array contents are also }); }); }); - -test('batches adds/removals', function(assert) { - assert.expect(23); - - let env = setupStore({ person: Person }); - let store = env.store; - let query1 = {} - - let payload = [ - { id: '1', name: 'Scumbag Dale' }, - { id: '2', name: 'Scumbag Katz' } - ]; - // resemble server side filtering - env.adapter.query = function(store, type, query, recordArray) { - return payload; - }; - - let result = Ember.run(() => { - return store.query('person', query1); - }); - - let arrayDidChange = 0; - let contentDidChange = 0; - - let lastRecordArray; - return result.then(recordArray => { - lastRecordArray = recordArray; - assert.deepEqual(recordArray.map(x => x.get('name')), ['Scumbag Dale', 'Scumbag Katz']); - - assert.equal(arrayDidChange, 0, 'array should not yet have emitted a change event'); - assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); - - recordArray.addObserver('content', function() { - contentDidChange++; - }); - - recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) { - arrayDidChange++; - - // first time invoked - assert.equal(array, recordArray, 'should be same record array as above'); - assert.equal(startIdx, 0, 'expected startIdx'); - assert.equal(removeAmt, 2, 'expcted removeAmt'); - assert.equal(addAmt, 2, 'expected addAmt'); - }); - - // set next payload; - payload = [ - { id: '3', name: 'Scumbag Penner' }, - { id: '4', name: 'Scumbag Hamilton' } - ]; - - return Ember.run(() => { - // re-query - let result = recordArray.update(); - assert.equal(arrayDidChange, 0); - assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); - return result; - }); - }).then(recordArray => { - assert.equal(arrayDidChange, 1, 'record array should have omitted ONE change event'); - assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); - assert.equal(recordArray, lastRecordArray); - - lastRecordArray = recordArray; - - assert.deepEqual(recordArray.map(x => x.get('name')), ['Scumbag Penner', 'Scumbag Hamilton']); - - // set next payload; - payload = [ - { id: '3', name: 'Scumbag Penner' } - ]; - - arrayDidChange = 0; // reset change event counter - contentDidChange = 0; // reset change event counter - - recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) { - arrayDidChange++; - - // first time invoked - assert.equal(array, recordArray, 'should be same recordArray as above'); - assert.equal(startIdx, 0, 'expected startIdx'); - assert.equal(removeAmt, 2, 'expcted removeAmt'); - assert.equal(addAmt, 1, 'expected addAmt'); - }); - - return Ember.run(() => { - // re-query - let result = recordArray.update(); - assert.equal(arrayDidChange, 0, 'record array should not yet have omitted a change event'); - assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); - return result; - }); - }).then(recordArray => { - assert.equal(arrayDidChange, 1, 'record array should have emitted one change event'); - assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); - assert.equal(recordArray, lastRecordArray); - - lastRecordArray = recordArray; - - assert.deepEqual(recordArray.map(x => x.get('name')), ['Scumbag Penner']); - }); -}); diff --git a/tests/unit/record-arrays/adapter-populated-record-array-test.js b/tests/unit/record-arrays/adapter-populated-record-array-test.js index c8f2c785ab2..d5d742377c7 100644 --- a/tests/unit/record-arrays/adapter-populated-record-array-test.js +++ b/tests/unit/record-arrays/adapter-populated-record-array-test.js @@ -8,12 +8,26 @@ const { AdapterPopulatedRecordArray } = DS; module('unit/record-arrays/adapter-populated-record-array - DS.AdapterPopulatedRecordArray'); +function recordFor(record) { + let _internalModel = { + getRecord() { + record._internalModel = _internalModel + return record; + } + } + + return { + id: record.id, + _internalModel + }; +} + test('default initial state', function(assert) { let recordArray = AdapterPopulatedRecordArray.create({ type: 'recordType' }); assert.equal(recordArray.get('isLoaded'), false, 'expected isLoaded to be false'); assert.equal(recordArray.get('type'), 'recordType'); - assert.equal(recordArray.get('content'), null); + assert.deepEqual(recordArray.get('content'), []); assert.equal(recordArray.get('query'), null); assert.equal(recordArray.get('store'), null); assert.equal(recordArray.get('links'), null); @@ -106,15 +120,8 @@ test('#loadRecords', function(assert) { manager }); - let model1 = { - id: 2, - _internalModel: { getRecord() { return model1; }} - }; - - let model2 = { - id: 2, - _internalModel: { getRecord() { return model2; }} - }; + let model1 = recordFor({ id: 1 }); + let model2 = recordFor({ id: 2 }); assert.equal(didAddRecord, 0, 'no records should have been added yet'); @@ -145,3 +152,118 @@ test('#loadRecords', function(assert) { }); assert.equal(didLoad, 1, 'didLoad event should have fired once'); }); + +test('change events when receiving a new query payload', function(assert) { + assert.expect(37); + + let arrayDidChange = 0; + let contentDidChange = 0; + let didAddRecord = 0; + + const manager = { + recordArraysForRecord(record) { + return { + add(array) { + didAddRecord++; + assert.equal(array, recordArray); + } + } + } + }; + + let recordArray = AdapterPopulatedRecordArray.create({ + query: 'some-query', + manager + }); + + run(() => { + recordArray.loadRecords([ + recordFor({ id: '1', name: 'Scumbag Dale' }), + recordFor({ id: '2', name: 'Scumbag Katz' }) + ], {}); + }); + + assert.equal(didAddRecord, 2, 'expected 2 didAddRecords'); + assert.deepEqual(recordArray.map(x => x.name), ['Scumbag Dale', 'Scumbag Katz']); + + assert.equal(arrayDidChange, 0, 'array should not yet have emitted a change event'); + assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); + + recordArray.addObserver('content', function() { + contentDidChange++; + }); + + recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) { + arrayDidChange++; + + // first time invoked + assert.equal(array, recordArray, 'should be same record array as above'); + assert.equal(startIdx, 0, 'expected startIdx'); + assert.equal(removeAmt, 2, 'expcted removeAmt'); + assert.equal(addAmt, 2, 'expected addAmt'); + }); + + assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); + assert.equal(recordArray.get('isUpdating'), false, 'should not yet be updating'); + + assert.equal(arrayDidChange, 0); + assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); + + arrayDidChange = 0; + contentDidChange = 0; + didAddRecord = 0; + + run(() => { + // re-query + recordArray.loadRecords([ + recordFor({ id: '3', name: 'Scumbag Penner' }), + recordFor({ id: '4', name: 'Scumbag Hamilton' }) + ], {}); + }); + + assert.equal(didAddRecord, 2, 'expected 2 didAddRecords'); + assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); + assert.equal(recordArray.get('isUpdating'), false, 'should no longer be updating'); + + assert.equal(arrayDidChange, 1, 'record array should have omitted ONE change event'); + assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); + + assert.deepEqual(recordArray.map(x => x.name), ['Scumbag Penner', 'Scumbag Hamilton']); + + arrayDidChange = 0; // reset change event counter + contentDidChange = 0; // reset change event counter + didAddRecord = 0; + + recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) { + arrayDidChange++; + + // first time invoked + assert.equal(array, recordArray, 'should be same recordArray as above'); + assert.equal(startIdx, 0, 'expected startIdx'); + assert.equal(removeAmt, 2, 'expcted removeAmt'); + assert.equal(addAmt, 1, 'expected addAmt'); + }); + + // re-query + assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); + assert.equal(recordArray.get('isUpdating'), false, 'should not yet be updating'); + + assert.equal(arrayDidChange, 0, 'record array should not yet have omitted a change event'); + assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); + + run(() => { + recordArray.loadRecords([ + recordFor({ id: '3', name: 'Scumbag Penner' }) + ], {}); + }); + + assert.equal(didAddRecord, 1, 'expected 1 didAddRecord'); + + assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); + assert.equal(recordArray.get('isUpdating'), false, 'should not longer be updating'); + + assert.equal(arrayDidChange, 1, 'record array should have emitted one change event'); + assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); + + assert.deepEqual(recordArray.map(x => x.name), ['Scumbag Penner']); +});