Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ON_HOLD] feat: add additional state-machine states to handle unload/loading edge cases #7475

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
});

Expand Down Expand Up @@ -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');
Expand Down
111 changes: 85 additions & 26 deletions packages/-ember-data/tests/integration/records/load-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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');
Expand All @@ -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');
});
});
6 changes: 3 additions & 3 deletions packages/store/addon/-private/system/core-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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
Expand Down
14 changes: 11 additions & 3 deletions packages/store/addon/-private/system/model/internal-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 => {
Expand Down
28 changes: 25 additions & 3 deletions packages/store/addon/-private/system/model/states.js
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ const updatedState = dirtyState({
});

function createdStateDeleteRecord(internalModel) {
internalModel.transitionTo('deleted.saved');
internalModel.transitionTo('deleted.saved.new');
internalModel.send('invokeLifecycleCallbacks');
}

Expand Down Expand Up @@ -469,6 +469,7 @@ const RootState = {
isDeleted: false,
isNew: false,
isValid: true,
isPreloaded: false,

// DEFAULT EVENTS

Expand All @@ -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) {
Expand Down Expand Up @@ -529,6 +539,14 @@ const RootState = {
internalModel._promiseProxy = null;
},

empty: {
isEmpty: true,
},

preloaded: {
isPreloaded: true,
},

loadingData() {},

// EVENTS
Expand Down Expand Up @@ -697,8 +715,12 @@ const RootState = {
// FLAGS
isDirty: false,

new: {
isNew: true,
},

setup(internalModel) {
internalModel.removeFromInverseRelationships();
internalModel.removeFromInverseRelationships(internalModel.currentState.isNew);
},

invokeLifecycleCallbacks(internalModel) {
Expand Down