fix and add more tests

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) {

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(

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;
 }

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;

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);