From 5a75092c7321a98eb72c8db2540c2f7aac7e710d Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 18 Aug 2022 12:06:38 -0700 Subject: [PATCH] fix and add more tests --- .../tests/integration/store-test.js | 83 +++++++++++++++++-- .../-private/legacy-relationships-support.ts | 2 +- .../addon/-private/caches/instance-cache.ts | 13 ++- .../addon/-private/network/fetch-manager.ts | 2 +- .../store/addon/-private/store-service.ts | 10 ++- 5 files changed, 93 insertions(+), 17 deletions(-) diff --git a/packages/-ember-data/tests/integration/store-test.js b/packages/-ember-data/tests/integration/store-test.js index 07a299f3d44..33e36c2eda6 100644 --- a/packages/-ember-data/tests/integration/store-test.js +++ b/packages/-ember-data/tests/integration/store-test.js @@ -279,6 +279,7 @@ module('integration/store - findRecord', function (hooks) { let store; hooks.beforeEach(function () { + this.owner.register('model:person', Person); this.owner.register('model:car', Car); this.owner.register('adapter:application', RESTAdapter.extend()); this.owner.register('serializer:application', RESTSerializer.extend()); @@ -351,18 +352,71 @@ module('integration/store - findRecord', function (hooks) { assert.strictEqual(car.model, 'Princess', 'Updated car record is returned'); }); - test('multiple calls to store#findRecord return the cached record without firing new requests', async function (assert) { - assert.expect(3); + test('multiple calls to store#findRecord return the cached record without waiting for background requests', async function (assert) { + assert.expect(6); this.owner.unregister('serializer:application'); let adapter = store.adapterFor('application'); let calls = 0; - adapter.shouldReloadRecord = () => false; - adapter.shouldBackgroundReloadRecord = () => false; + delete adapter.shouldReloadRecord; + adapter.shouldBackgroundReloadRecord = () => true; adapter.findRecord = () => { - if (calls++ === 0) { + if (calls++ < 3) { + return resolve({ + data: { + type: 'car', + id: '1', + attributes: { + make: 'BMC', + model: 'Mini', + }, + }, + }); + } + assert.ok(false, 'should not call findRecord'); + throw 'unexpected call'; + }; + + const proxiedCar = store.findRecord('car', '1'); + const car = await proxiedCar; + + assert.strictEqual(car.model, 'Mini', 'car record is returned from cache'); + const proxiedCar2 = store.findRecord('car', '1'); // will trigger a backgroundReload + const car2 = await proxiedCar2; + + assert.strictEqual(car2?.model, 'Mini', 'car record is returned from cache'); + assert.strictEqual(car, car2, 'we found the same car'); + + const proxiedCar3 = store.findRecord('car', '1'); // will trigger a backgroundReload + const car3 = await proxiedCar3; + + assert.strictEqual(car3?.model, 'Mini', 'car record is returned from cache'); + assert.strictEqual(car, car3, 'we found the same car'); + + await settled(); + + assert.strictEqual(calls, 3, 'we triggered one background reload and one load'); + }); + + test('multiple parallel calls to store#findRecord return the cached record without waiting for background requests', async function (assert) { + assert.expect(8); + + this.owner.unregister('serializer:application'); + let adapter = store.adapterFor('application'); + let calls = 0; + + delete adapter.shouldReloadRecord; + adapter.shouldBackgroundReloadRecord = () => true; + + function timeout(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); + } + + adapter.findRecord = async () => { + await timeout(1); + if (calls++ < 2) { return resolve({ data: { type: 'car', @@ -382,13 +436,26 @@ module('integration/store - findRecord', function (hooks) { const car = await proxiedCar; assert.strictEqual(car.model, 'Mini', 'car record is returned from cache'); - const proxiedCar2 = store.findRecord('car', '1'); - assert.strictEqual(proxiedCar2.get('model'), undefined, 'car record is returned from cache'); - // assert.strictEqual(proxiedCar2.get('model'), 'Mini', 'car record is returned from cache'); + const proxiedCar2 = store.findRecord('car', '1'); // will trigger a backgroundReload + const proxiedCar3 = store.findRecord('car', '1'); // will not trigger a backgroundReload + const proxiedCar4 = store.findRecord('car', '1'); // will not trigger a backgroundReload + const car2 = await proxiedCar2; + const car3 = await proxiedCar3; + const car4 = await proxiedCar4; assert.strictEqual(car2?.model, 'Mini', 'car record is returned from cache'); assert.strictEqual(car, car2, 'we found the same car'); + + assert.strictEqual(car3?.model, 'Mini', 'car record is returned from cache'); + assert.strictEqual(car, car3, 'we found the same car'); + + assert.strictEqual(car4?.model, 'Mini', 'car record is returned from cache'); + assert.strictEqual(car, car4, 'we found the same car'); + + await settled(); + + assert.strictEqual(calls, 2, 'we triggered one background reload and one load'); }); test('store#findRecord { reload: true } ignores cached record and reloads record from server', async function (assert) { diff --git a/packages/model/addon/-private/legacy-relationships-support.ts b/packages/model/addon/-private/legacy-relationships-support.ts index 243d76e4718..f26f0624a51 100644 --- a/packages/model/addon/-private/legacy-relationships-support.ts +++ b/packages/model/addon/-private/legacy-relationships-support.ts @@ -704,7 +704,7 @@ function areAllInverseRecordsLoaded(store: Store, resource: JsonApiRelationship) function isEmpty(store: Store, cache: IdentifierCache, resource: ResourceIdentifierObject): boolean { const identifier = cache.getOrCreateRecordIdentifier(resource); const recordData = store._instanceCache.peek({ identifier, bucket: 'recordData' }); - return !recordData || !!recordData.isEmpty?.(identifier); + return !recordData || recordData.isEmpty(identifier); } function isBelongsTo( diff --git a/packages/store/addon/-private/caches/instance-cache.ts b/packages/store/addon/-private/caches/instance-cache.ts index ed62be98229..ce6a2553341 100644 --- a/packages/store/addon/-private/caches/instance-cache.ts +++ b/packages/store/addon/-private/caches/instance-cache.ts @@ -139,7 +139,7 @@ export class InstanceCache { return false; } const isNew = recordData.isNew(identifier); - const isEmpty = recordData.isEmpty?.(identifier) || false; + const isEmpty = recordData.isEmpty(identifier); // if we are new we must consider ourselves loaded if (isNew) { @@ -152,18 +152,23 @@ export class InstanceCache { // we should consider allowing for something to be loaded that is simply "not empty". // which is how RecordState currently handles this case; however, RecordState is buggy // in that it does not account for unloading. - // return !isEmpty; + return filterDeleted && recordData.isDeletionCommitted(identifier) ? false : !isEmpty; + /* const req = this.store.getRequestStateService(); const fulfilled = req.getLastRequestForRecord(identifier); + const isLocallyLoaded = !isEmpty; const isLoading = - fulfilled !== null && req.getPendingRequestsForRecord(identifier).some((req) => req.type === 'query'); + !isLocallyLoaded && + fulfilled === null && + req.getPendingRequestsForRecord(identifier).some((req) => req.type === 'query'); if (isEmpty || (filterDeleted && recordData.isDeletionCommitted(identifier)) || isLoading) { return false; } return true; + */ } constructor(store: Store) { @@ -684,7 +689,7 @@ function _isEmpty(cache: InstanceCache, identifier: StableRecordIdentifier): boo } const isNew = recordData.isNew(identifier); const isDeleted = recordData.isDeleted(identifier); - const isEmpty = recordData.isEmpty?.(identifier) || false; + const isEmpty = recordData.isEmpty(identifier); return (!isNew || isDeleted) && isEmpty; } diff --git a/packages/store/addon/-private/network/fetch-manager.ts b/packages/store/addon/-private/network/fetch-manager.ts index db02934f7f5..2cb3e10846d 100644 --- a/packages/store/addon/-private/network/fetch-manager.ts +++ b/packages/store/addon/-private/network/fetch-manager.ts @@ -213,7 +213,7 @@ export default class FetchManager { }, (error) => { const recordData = store._instanceCache.peek({ identifier, bucket: 'recordData' }); - if (!recordData || recordData.isEmpty?.(identifier) || isLoading) { + if (!recordData || recordData.isEmpty(identifier) || isLoading) { store._instanceCache.unloadRecord(identifier); } throw error; diff --git a/packages/store/addon/-private/store-service.ts b/packages/store/addon/-private/store-service.ts index ace70bca8e4..030efd12d19 100644 --- a/packages/store/addon/-private/store-service.ts +++ b/packages/store/addon/-private/store-service.ts @@ -63,6 +63,7 @@ import NotificationManager from './managers/record-notification-manager'; import FetchManager, { SaveOp } from './network/fetch-manager'; import { _findAll, _query, _queryRecord } from './network/finders'; import type RequestCache from './network/request-cache'; +import type Snapshot from './network/snapshot'; import { PromiseArray, promiseArray, PromiseObject, promiseObject } from './proxies/promise-proxies'; import AdapterPopulatedRecordArray from './record-arrays/adapter-populated-record-array'; import RecordArray from './record-arrays/record-array'; @@ -1079,14 +1080,14 @@ class Store extends Service { assertIdentifierHasId(identifier); promise = this._fetchManager.scheduleFetch(identifier, options); } else { - let snapshot = this._instanceCache.createSnapshot(identifier, options); + let snapshot: Snapshot | null = null; let adapter = this.adapterFor(identifier.type); // Refetch the record if the adapter thinks the record is stale if ( typeof options.reload === 'undefined' && adapter.shouldReloadRecord && - adapter.shouldReloadRecord(this, snapshot) + adapter.shouldReloadRecord(this, (snapshot = this._instanceCache.createSnapshot(identifier, options))) ) { assertIdentifierHasId(identifier); promise = this._fetchManager.scheduleFetch(identifier, options); @@ -1096,7 +1097,10 @@ class Store extends Service { options.backgroundReload !== false && (options.backgroundReload || !adapter.shouldBackgroundReloadRecord || - adapter.shouldBackgroundReloadRecord(this, snapshot)) + adapter.shouldBackgroundReloadRecord( + this, + (snapshot = snapshot || this._instanceCache.createSnapshot(identifier, options)) + )) ) { assertIdentifierHasId(identifier); this._fetchManager.scheduleFetch(identifier, options);