From 9ffe5bfc6dab78879025b19b46b2f1066ea8eac1 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 21 Apr 2021 01:37:34 -0700 Subject: [PATCH] feat: add additional state-machine states to handle unload/loading edge cases --- .../integration/records/delete-record-test.js | 45 ++++++- .../tests/integration/records/load-test.js | 111 ++++++++++++++---- .../store/addon/-private/system/core-store.ts | 6 +- .../-private/system/model/internal-model.ts | 14 ++- .../addon/-private/system/model/states.js | 28 ++++- 5 files changed, 167 insertions(+), 37 deletions(-) diff --git a/packages/-ember-data/tests/integration/records/delete-record-test.js b/packages/-ember-data/tests/integration/records/delete-record-test.js index 98edea59a5a..308c7c1c800 100644 --- a/packages/-ember-data/tests/integration/records/delete-record-test.js +++ b/packages/-ember-data/tests/integration/records/delete-record-test.js @@ -2,6 +2,7 @@ import { get } from '@ember/object'; import { run } from '@ember/runloop'; +import settled from '@ember/test-helpers/settled'; import { module, test } from 'qunit'; import { all, Promise as EmberPromise } from 'rsvp'; @@ -223,7 +224,7 @@ module('integration/deletedRecord - Deleting Records', function(hooks) { record.deleteRecord(); }); - assert.equal(get(record, 'currentState.stateName'), 'root.deleted.saved'); + assert.equal(get(record, 'currentState.stateName'), 'root.deleted.saved.new'); assert.equal(get(store.peekAll('person'), 'length'), 0, 'The new person should be removed from the store'); }); @@ -269,10 +270,50 @@ module('integration/deletedRecord - Deleting Records', function(hooks) { record.destroyRecord(); }); - assert.equal(get(record, 'currentState.stateName'), 'root.deleted.saved'); + assert.equal(get(record, 'currentState.stateName'), 'root.deleted.saved.new'); assert.equal(get(store.peekAll('person'), 'length'), 0, 'The new person should be removed from the store'); }); + test('unloading a deleted persisted record should transition it to root.empty.deleted', async function(assert) { + class TestAdapter { + static create() { + return new this(); + } + + deleteRecord() { + return { data: null }; + } + } + this.owner.register('adapter:person', TestAdapter); + const store = this.owner.lookup('service:store'); + const person = store.push({ + data: { + type: 'person', + id: '1', + attributes: { name: 'Rosemary Sweet Thoburn' }, + }, + }); + const im = person._internalModel; + person.deleteRecord(); + assert.true(person.currentState.isDeleted, 'we are deleted before the save'); + assert.equal(person.currentState.stateName, 'root.deleted.uncommitted', 'our deletion is uncommitted'); + let promise = person.save(); + + assert.true(person.currentState.isDeleted, 'we are deleted during the save'); + assert.equal(person.currentState.stateName, 'root.deleted.inFlight', 'our deletion is in-flight'); + await promise; + assert.true(person.currentState.isDeleted, 'we are deleted after the save'); + assert.equal(person.currentState.stateName, 'root.deleted.saved', 'our deletion was persisted'); + person.unloadRecord(); + await settled(); + assert.true(person.currentState.isDeleted, 'we are deleted after the unload'); + assert.equal(person.currentState.stateName, 'root.deleted.saved', 'the record reflects the deleted state'); + + // destroy divorces the state of the internal-model from the state of the record. + assert.true(im.currentState.isDeleted, 'we are internally deleted after the unload'); + assert.equal(im.currentState.stateName, 'root.empty.deleted', 'the internal state is empty.deleted'); + }); + test('Will resolve destroy and save in same loop', function(assert) { let store = this.owner.lookup('service:store'); let adapter = store.adapterFor('application'); diff --git a/packages/-ember-data/tests/integration/records/load-test.js b/packages/-ember-data/tests/integration/records/load-test.js index 507176d2956..5b911424a32 100644 --- a/packages/-ember-data/tests/integration/records/load-test.js +++ b/packages/-ember-data/tests/integration/records/load-test.js @@ -9,7 +9,6 @@ import JSONAPIAdapter from '@ember-data/adapter/json-api'; import Model, { attr, belongsTo } from '@ember-data/model'; import JSONAPISerializer from '@ember-data/serializer/json-api'; import Store from '@ember-data/store'; -import todo from '@ember-data/unpublished-test-infra/test-support/todo'; class Person extends Model { @attr() @@ -45,7 +44,7 @@ module('integration/load - Loading Records', function(hooks) { }); }); - todo('Empty records remain in the empty state while data is being fetched', async function(assert) { + test('Empty records remain in the empty state while data is being fetched', async function(assert) { let payloads = [ { data: { @@ -123,23 +122,23 @@ module('integration/load - Loading Records', function(hooks) { let internalModel = store._internalModelForId('person', '1'); // test that our initial state is correct - assert.equal(internalModel.currentState.isEmpty, true, 'We begin in the empty state'); - assert.equal(internalModel.currentState.isLoading, false, 'We have not triggered a load'); - assert.equal(internalModel.isReloading, false, 'We are not reloading'); + assert.true(internalModel.currentState.isEmpty, 'We begin in the empty state'); + assert.false(internalModel.currentState.isLoading, 'We have not triggered a load'); + assert.false(internalModel.isReloading, 'We are not reloading'); let recordPromise = store.findRecord('person', '1'); // test that during the initial load our state is correct - assert.todo.equal(internalModel.currentState.isEmpty, true, 'awaiting first fetch: We remain in the empty state'); - assert.equal(internalModel.currentState.isLoading, true, 'awaiting first fetch: We have now triggered a load'); - assert.equal(internalModel.isReloading, false, 'awaiting first fetch: We are not reloading'); + assert.true(internalModel.currentState.isEmpty, 'awaiting first fetch: We remain in the empty state'); + assert.true(internalModel.currentState.isLoading, 'awaiting first fetch: We have now triggered a load'); + assert.false(internalModel.isReloading, 'awaiting first fetch: We are not reloading'); let record = await recordPromise; // test that after the initial load our state is correct - assert.equal(internalModel.currentState.isEmpty, false, 'after first fetch: We are no longer empty'); - assert.equal(internalModel.currentState.isLoading, false, 'after first fetch: We have loaded'); - assert.equal(internalModel.isReloading, false, 'after first fetch: We are not reloading'); + assert.false(internalModel.currentState.isEmpty, 'after first fetch: We are no longer empty'); + assert.false(internalModel.currentState.isLoading, 'after first fetch: We have loaded'); + assert.false(internalModel.isReloading, 'after first fetch: We are not reloading'); let bestFriend = await record.get('bestFriend'); let trueBestFriend = await bestFriend.get('bestFriend'); @@ -155,37 +154,97 @@ module('integration/load - Loading Records', function(hooks) { recordPromise = record.reload(); // test that during a reload our state is correct - assert.equal(internalModel.currentState.isEmpty, false, 'awaiting reload: We remain non-empty'); - assert.equal(internalModel.currentState.isLoading, false, 'awaiting reload: We are not loading again'); - assert.equal(internalModel.isReloading, true, 'awaiting reload: We are reloading'); + assert.false(internalModel.currentState.isEmpty, 'awaiting reload: We remain non-empty'); + assert.false(internalModel.currentState.isLoading, 'awaiting reload: We are not loading again'); + assert.true(internalModel.isReloading, 'awaiting reload: We are reloading'); await recordPromise; // test that after a reload our state is correct - assert.equal(internalModel.currentState.isEmpty, false, 'after reload: We remain non-empty'); - assert.equal(internalModel.currentState.isLoading, false, 'after reload: We have loaded'); - assert.equal(internalModel.isReloading, false, 'after reload:: We are not reloading'); + assert.false(internalModel.currentState.isEmpty, 'after reload: We remain non-empty'); + assert.false(internalModel.currentState.isLoading, 'after reload: We have loaded'); + assert.false(internalModel.isReloading, 'after reload:: We are not reloading'); run(() => record.unloadRecord()); // test that after an unload our state is correct - assert.equal(internalModel.currentState.isEmpty, true, 'after unload: We are empty again'); - assert.equal(internalModel.currentState.isLoading, false, 'after unload: We are not loading'); - assert.equal(internalModel.isReloading, false, 'after unload:: We are not reloading'); + assert.true(internalModel.currentState.isEmpty, 'after unload: We are empty again'); + assert.false(internalModel.currentState.isLoading, 'after unload: We are not loading'); + assert.false(internalModel.isReloading, 'after unload:: We are not reloading'); recordPromise = store.findRecord('person', '1'); // test that during a reload-due-to-unload our state is correct // This requires a retainer (the async bestFriend relationship) - assert.todo.equal(internalModel.currentState.isEmpty, true, 'awaiting second find: We remain empty'); - assert.equal(internalModel.currentState.isLoading, true, 'awaiting second find: We are loading again'); - assert.equal(internalModel.isReloading, false, 'awaiting second find: We are not reloading'); + assert.true(internalModel.currentState.isEmpty, 'awaiting second find: We remain empty'); + assert.true(internalModel.currentState.isLoading, 'awaiting second find: We are loading again'); + assert.false(internalModel.isReloading, 'awaiting second find: We are not reloading'); await recordPromise; // test that after the reload-due-to-unload our state is correct - assert.equal(internalModel.currentState.isEmpty, false, 'after second find: We are no longer empty'); - assert.equal(internalModel.currentState.isLoading, false, 'after second find: We have loaded'); - assert.equal(internalModel.isReloading, false, 'after second find: We are not reloading'); + assert.false(internalModel.currentState.isEmpty, 'after second find: We are no longer empty'); + assert.false(internalModel.currentState.isLoading, 'after second find: We have loaded'); + assert.false(internalModel.isReloading, 'after second find: We are not reloading'); + }); + + test('Preloaded records do not remain in the empty state while data is being fetched', async function(assert) { + let payloads = [ + { + data: { + type: 'person', + id: '1', + attributes: { name: 'Chris' }, + relationships: {}, + }, + included: [], + }, + ]; + + this.owner.register( + 'adapter:application', + JSONAPIAdapter.extend({ + findRecord() { + let payload = payloads.shift(); + + if (payload === undefined) { + return reject(new Error('Invalid Request')); + } + + return resolve(payload); + }, + }) + ); + this.owner.register( + 'serializer:application', + JSONAPISerializer.extend({ + normalizeResponse(_, __, data) { + return data; + }, + }) + ); + + let internalModel = store._internalModelForId('person', '1'); + + // test that our initial state is correct + assert.true(internalModel.currentState.isEmpty, 'We begin in the empty state'); + assert.false(internalModel.currentState.isLoading, 'We have not triggered a load'); + assert.false(internalModel.isReloading, 'We are not reloading'); + + let recordPromise = store.findRecord('person', '1', { preload: { name: 'Chris' } }); + + // test that during the initial load our state is correct + assert.false(internalModel.currentState.isEmpty, 'awaiting first fetch: We are no longer in the empty state'); + assert.true(internalModel.currentState.isPreloaded, 'awaiting first fetch: We are in a preloaded state'); + assert.true(internalModel.currentState.isLoading, 'awaiting first fetch: We have now triggered a load'); + assert.false(internalModel.isReloading, 'awaiting first fetch: We are not reloading'); + + await recordPromise; + + // test that after the initial load our state is correct + assert.false(internalModel.currentState.isEmpty, 'after first fetch: We are no longer empty'); + assert.false(internalModel.currentState.isPreloaded, 'awaiting first fetch: We are no longer in a preloaded state'); + assert.false(internalModel.currentState.isLoading, 'after first fetch: We have loaded'); + assert.false(internalModel.isReloading, 'after first fetch: We are not reloading'); }); }); diff --git a/packages/store/addon/-private/system/core-store.ts b/packages/store/addon/-private/system/core-store.ts index 113ed981065..f71c6aeb98e 100644 --- a/packages/store/addon/-private/system/core-store.ts +++ b/packages/store/addon/-private/system/core-store.ts @@ -2569,7 +2569,7 @@ abstract class CoreStore extends Service { operation = 'updateRecord'; } } else { - if (internalModel.currentState.stateName === 'root.deleted.saved') { + if (internalModel.currentState.stateName.indexOf('root.deleted.saved') === 0) { resolver.resolve(); continue; } else if (internalModel.isNew()) { @@ -2698,9 +2698,9 @@ abstract class CoreStore extends Service { let internalModel = internalModelFactoryFor(this).lookup(resource, data); // store.push will be from empty - // findRecord will be from root.loading + // findRecord will be from root.loading.{empty,preloaded} // all else will be updates - const isLoading = internalModel.currentState.stateName === 'root.loading'; + const isLoading = internalModel.currentState.stateName.indexOf('root.loading') === 0; const isUpdate = internalModel.currentState.isEmpty === false && !isLoading; // exclude store.push (root.empty) case diff --git a/packages/store/addon/-private/system/model/internal-model.ts b/packages/store/addon/-private/system/model/internal-model.ts index bf273995def..c332ea51629 100644 --- a/packages/store/addon/-private/system/model/internal-model.ts +++ b/packages/store/addon/-private/system/model/internal-model.ts @@ -276,7 +276,7 @@ export default class InternalModel { if (RECORD_DATA_STATE) { isRecordFullyDeleted = this._isRecordFullyDeleted(); } else { - isRecordFullyDeleted = this.currentState.stateName === 'root.deleted.saved'; + isRecordFullyDeleted = this.currentState.stateName.indexOf('root.deleted.saved') === 0; } return this._isDematerializing || this.hasScheduledDestroy() || this.isDestroyed || isRecordFullyDeleted; } @@ -293,7 +293,7 @@ export default class InternalModel { ) { return true; } else { - return this.currentState.stateName === 'root.deleted.saved'; + return this.currentState.stateName.indexOf('root.deleted.saved') === 0; } } else { // assert here @@ -446,7 +446,12 @@ export default class InternalModel { this._record = null; this.isReloading = false; this.error = null; - this.currentState = RootState.empty; + + if (this.currentState.isDeleted && !this.currentState.isDirty) { + this.currentState = RootState.empty.deleted; + } else { + this.currentState = RootState.empty; + } } deleteRecord() { @@ -1204,6 +1209,9 @@ export default class InternalModel { @param {Object} preload */ preloadData(preload) { + if (this.currentState.isEmpty) { + this.transitionTo('empty.preloaded'); + } let jsonPayload: JsonApiResource = {}; //TODO(Igor) consider the polymorphic case Object.keys(preload).forEach(key => { diff --git a/packages/store/addon/-private/system/model/states.js b/packages/store/addon/-private/system/model/states.js index d7e0198fbf6..c897e65a16e 100644 --- a/packages/store/addon/-private/system/model/states.js +++ b/packages/store/addon/-private/system/model/states.js @@ -416,7 +416,7 @@ const updatedState = dirtyState({ }); function createdStateDeleteRecord(internalModel) { - internalModel.transitionTo('deleted.saved'); + internalModel.transitionTo('deleted.saved.new'); internalModel.send('invokeLifecycleCallbacks'); } @@ -469,6 +469,7 @@ const RootState = { isDeleted: false, isNew: false, isValid: true, + isPreloaded: false, // DEFAULT EVENTS @@ -491,12 +492,21 @@ const RootState = { empty: { isEmpty: true, + deleted: { + isDeleted: true, + }, + + preloaded: { + isPreloaded: true, + }, + // EVENTS loadingData(internalModel, promise) { if (!REQUEST_SERVICE) { internalModel._promiseProxy = promise; } - internalModel.transitionTo('loading'); + // internalModel.transitionTo('loading'); + internalModel.transitionTo(this.isPreloaded ? 'loading.preloaded' : 'loading.empty'); }, loadedData(internalModel) { @@ -529,6 +539,14 @@ const RootState = { internalModel._promiseProxy = null; }, + empty: { + isEmpty: true, + }, + + preloaded: { + isPreloaded: true, + }, + loadingData() {}, // EVENTS @@ -697,8 +715,12 @@ const RootState = { // FLAGS isDirty: false, + new: { + isNew: true, + }, + setup(internalModel) { - internalModel.removeFromInverseRelationships(); + internalModel.removeFromInverseRelationships(internalModel.currentState.isNew); }, invokeLifecycleCallbacks(internalModel) {