Fix fetches that resolve before DataProvider commits

.changeset/precommit-fetch.md

@@ -0,0 +1,18 @@
+---
+'@data-client/core': patch
+'@data-client/endpoint': patch
+'@data-client/graphql': patch
+'@data-client/img': patch
+'@data-client/normalizr': patch
+'@data-client/react': patch
+'@data-client/rest': patch
+'@data-client/test': patch
+'@data-client/vue': patch
+'@data-client/use-enhanced-reducer': patch
+---
+
+Fix Suspense staying on the fallback when a fetch resolves before the store commits
+
+A [useSuspense](/docs/api/useSuspense) or `use(useFetch())` read that finishes while [DataProvider](/docs/api/DataProvider) is still rendering now shows its result once the provider commits, instead of leaving the fallback up. The endpoint is not called again. A fetch that fails is included.
+
+[useEnhancedReducer](https://www.npmjs.com/package/@data-client/use-enhanced-reducer) applies actions that arrive before the hook has committed, in order, from its mount effect. `dispatch` still returns a promise that resolves when that action commits.

packages/core/src/manager/NetworkManager.ts

@@ -24,6 +24,38 @@ export interface FetchingMeta {
   fetchedAt: number;
 }
 
+/** Result of a throttled fetch, and the controller it was resolved into. */
+interface SettledFetch {
+  controller: Controller;
+  endpoint: FetchAction['endpoint'];
+  args: FetchAction['args'];
+  response: unknown;
+  fetchedAt: number;
+  error: boolean;
+}
+
+const settledByMeta = new WeakMap<FetchingMeta, SettledFetch>();
+
+function noteSettled(
+  fetching: Map<string, FetchingMeta>,
+  controller: Controller,
+  action: FetchAction,
+  fetchedAt: number,
+  response: unknown,
+  error: boolean,
+) {
+  const meta = fetching.get(action.key);
+  if (!meta || meta.fetchedAt !== fetchedAt) return;
+  settledByMeta.set(meta, {
+    controller,
+    endpoint: action.endpoint,
+    args: action.args,
+    response,
+    fetchedAt,
+    error,
+  });
+}
+
 /** Handles all async network dispatches
  *
  * Dedupes concurrent requests by keeping track of all fetches in flight
@@ -105,6 +137,27 @@ export default class NetworkManager implements Manager {
   /** On mount */
   init() {
     delete this.cleanupDate;
+    // A fetch can resolve into a store that never commits. Replay that result
+    // through the controller that just committed, without calling the endpoint.
+    for (const meta of this.fetching.values()) {
+      const settled = settledByMeta.get(meta);
+      if (!settled || settled.controller === this.controller) continue;
+      settled.controller = this.controller;
+      if (settled.error) {
+        this.controller.resolve(settled.endpoint, {
+          args: settled.args,
+          response: settled.response as Error,
+          fetchedAt: settled.fetchedAt,
+          error: true,
+        });
+      } else {
+        this.controller.resolve(settled.endpoint, {
+          args: settled.args,
+          response: settled.response,
+          fetchedAt: settled.fetchedAt,
+        });
+      }
+    }
   }
 
   /** Ensures all promises are completed by rejecting remaining. */
@@ -137,7 +190,9 @@ export default class NetworkManager implements Manager {
   /** Clear promise state for a given key */
   protected clear(key: string) {
     if (this.fetching.has(key)) {
-      (this.fetching.get(key) as FetchingMeta).promise.catch(() => {});
+      const meta = this.fetching.get(key) as FetchingMeta;
+      meta.promise.catch(() => {});
+      settledByMeta.delete(meta);
       this.fetching.delete(key);
     }
   }
@@ -193,6 +248,15 @@ export default class NetworkManager implements Manager {
 
           // don't update state with promises started before last clear
           if (fetchedAt >= lastReset) {
+            if (throttle)
+              noteSettled(
+                this.fetching,
+                this.controller,
+                action,
+                fetchedAt,
+                response,
+                false,
+              );
             this.controller.resolve(action.endpoint, {
               args: action.args,
               response,
@@ -205,6 +269,15 @@ export default class NetworkManager implements Manager {
           const lastReset = this.getLastReset();
           // don't update state with promises started before last clear
           if (fetchedAt >= lastReset) {
+            if (throttle)
+              noteSettled(
+                this.fetching,
+                this.controller,
+                action,
+                fetchedAt,
+                error,
+                true,
+              );
             this.controller.resolve(action.endpoint, {
               args: action.args,
               response: error,

packages/core/src/manager/__tests__/networkManager.ts

@@ -397,3 +397,94 @@ describe('NetworkManager', () => {
     });
   });
 });
+
+describe('NetworkManager re-resolves a fetch whose store never committed', () => {
+  function bind(dispatch: jest.Mock) {
+    return new Controller({ dispatch, getState: () => initialState });
+  }
+
+  async function start(nm: NetworkManager, endpoint: any, dispatch: jest.Mock) {
+    const controller = bind(dispatch);
+    const action = createFetch(endpoint, { args: [] });
+    void Promise.resolve(action.meta.promise).catch(() => {});
+    const pending = nm.middleware(controller)(jest.fn(() => Promise.resolve()))(
+      action,
+    );
+    await Promise.resolve(pending).catch(() => {});
+    await new Promise(resolve => setTimeout(resolve, 0));
+    return { controller, action };
+  }
+
+  it('does not resolve again when the same controller commits', async () => {
+    let calls = 0;
+    const endpoint = new Endpoint(
+      () => {
+        calls += 1;
+        return Promise.resolve(5);
+      },
+      { name: 'settledSame' },
+    );
+    const dispatch = jest.fn(() => Promise.resolve());
+    const nm = new NetworkManager();
+    await start(nm, endpoint, dispatch);
+    expect(calls).toBe(1);
+    expect(dispatch).toHaveBeenCalledTimes(1);
+    nm.init();
+    expect(dispatch).toHaveBeenCalledTimes(1);
+    expect(calls).toBe(1);
+    nm.cleanup();
+  });
+
+  it('re-resolves a response into the controller that commits', async () => {
+    let calls = 0;
+    const endpoint = new Endpoint(
+      () => {
+        calls += 1;
+        return Promise.resolve(5);
+      },
+      { name: 'settledValue' },
+    );
+    const first = jest.fn((_action: any) => Promise.resolve());
+    const second = jest.fn((_action: any) => Promise.resolve());
+    const nm = new NetworkManager();
+    await start(nm, endpoint, first);
+    nm.middleware(bind(second));
+    nm.init();
+    expect(calls).toBe(1);
+    expect(second).toHaveBeenCalledTimes(1);
+    expect(second.mock.calls[0][0]).toMatchObject({
+      type: SET_RESPONSE,
+      response: 5,
+      error: false,
+    });
+    nm.init();
+    expect(second).toHaveBeenCalledTimes(1);
+    nm.cleanup();
+  });
+
+  it('re-resolves an error into the controller that commits', async () => {
+    let calls = 0;
+    const failure = new Error('nope');
+    const endpoint = new Endpoint(
+      () => {
+        calls += 1;
+        return Promise.reject(failure);
+      },
+      { name: 'settledError' },
+    );
+    const first = jest.fn((_action: any) => Promise.resolve());
+    const second = jest.fn((_action: any) => Promise.resolve());
+    const nm = new NetworkManager();
+    await start(nm, endpoint, first);
+    nm.middleware(bind(second));
+    nm.init();
+    expect(calls).toBe(1);
+    expect(second).toHaveBeenCalledTimes(1);
+    expect(second.mock.calls[0][0]).toMatchObject({
+      type: SET_RESPONSE,
+      response: failure,
+      error: true,
+    });
+    nm.cleanup();
+  });
+});

packages/react/src/components/__tests__/precommit-race.native.tsx

@@ -0,0 +1,29 @@
+import type { ReactNode } from 'react';
+import { Text } from 'react-native';
+import TestRenderer from 'react-test-renderer';
+
+import {
+  registerPrecommitRaceTests,
+  type RaceHost,
+} from './precommit-race.node-suite';
+
+const host: RaceHost = {
+  Text: ({ children }: { children?: ReactNode }) => <Text>{children}</Text>,
+  createRenderer() {
+    let renderer: TestRenderer.ReactTestRenderer | undefined;
+    return {
+      render(node) {
+        if (!renderer) renderer = TestRenderer.create(node);
+        else renderer.update(node);
+      },
+      read() {
+        return JSON.stringify(renderer?.toJSON() ?? '');
+      },
+      unmount() {
+        renderer?.unmount();
+      },
+    };
+  },
+};
+
+registerPrecommitRaceTests(host);