diff --git a/.changeset/hold-precommit-dispatch.md b/.changeset/hold-precommit-dispatch.md new file mode 100644 index 000000000000..5004097b9e3f --- /dev/null +++ b/.changeset/hold-precommit-dispatch.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/use-enhanced-reducer': patch +'@data-client/vue': patch +--- + +Fix Suspense staying on the fallback when a resolved read races the first commit + +`useSuspense` of an endpoint that is already resolved now shows its value when React restarts `DataProvider` before that provider commits. The read still starts during render. + +A `managers` array kept outside `DataProvider` can still leave that Suspense fallback up when the response was delivered to a provider React discarded. Let `DataProvider` build the managers, or create them with the provider that commits. diff --git a/packages/react/src/components/__tests__/useSuspense-precommit.native.tsx b/packages/react/src/components/__tests__/useSuspense-precommit.native.tsx new file mode 100644 index 000000000000..e195e601087f --- /dev/null +++ b/packages/react/src/components/__tests__/useSuspense-precommit.native.tsx @@ -0,0 +1,145 @@ +import { Endpoint } from '@data-client/endpoint'; +import React, { Suspense, use, useState } from 'react'; +import { Text } from 'react-native'; +import TestRenderer from 'react-test-renderer'; + +import { DataProvider, useSuspense } from '../..'; +import { getDefaultManagers } from '../getDefaultManagers'; + +/** + * Same race as useSuspense-precommit.web.tsx, on the React Native renderer. + * `getState()` at dispatch-promise resolution is not asserted here: this + * renderer still returns the previous snapshot then, including on master. + */ +describe('useSuspense already-resolved endpoint before commit (native)', () => { + let renderer: TestRenderer.ReactTestRenderer | undefined; + let prevActEnv: boolean | undefined; + let errors: string[]; + let errorSpy: jest.SpyInstance; + + beforeEach(() => { + prevActEnv = (globalThis as any).IS_REACT_ACT_ENVIRONMENT; + (globalThis as any).IS_REACT_ACT_ENVIRONMENT = false; + errors = []; + errorSpy = jest.spyOn(console, 'error').mockImplementation((...args) => { + errors.push(args.map(String).join(' ')); + }); + }); + + afterEach(() => { + renderer?.unmount(); + renderer = undefined; + (globalThis as any).IS_REACT_ACT_ENVIRONMENT = prevActEnv; + errorSpy.mockRestore(); + }); + + function mount(element: React.ReactElement) { + return Promise.resolve().then(() => { + renderer = TestRenderer.create(element, { + unstable_isConcurrent: true, + } as unknown as TestRenderer.TestRendererOptions); + }); + } + + test('shows the resolved value once the commit flushes', async () => { + const observed = await renderRace({ park: 'none' }); + expect(observed.text).toContain('value 5'); + expect(observed.calls).toBeLessThanOrEqual(3); + expect(observed.warning).toBe(false); + }); + + test('a parked render with a fresh provider still resolves', async () => { + const observed = await renderRace({ park: 'outside' }); + expect(observed.text).toContain('value 5'); + expect(observed.calls).toBeLessThanOrEqual(5); + expect(observed.warning).toBe(false); + }); + + async function renderRace({ park }: { park: 'none' | 'outside' }) { + let calls = 0; + const endpoint = new Endpoint( + () => { + calls += 1; + if (calls > 30) return new Promise(() => undefined); + return Promise.resolve(5); + }, + { dataExpiryLength: Infinity }, + ); + const gate = deferred(); + + function Reader() { + const value = useSuspense(endpoint); + return value {String(value)}; + } + function Park() { + if (!gate.done) use(gate.promise); + return null; + } + function Store({ children }: { children: React.ReactNode }) { + const [managers] = useState(() => getDefaultManagers()); + return ( + + {children} + + ); + } + + await mount( + <> + + fallback}> + + + + {park === 'outside' ? + + : null} + , + ); + + await new Promise(resolve => setTimeout(resolve, 30)); + if (park !== 'none') { + gate.done = true; + gate.resolve(); + } + const text = await waitForText(() => treeText(renderer), 800); + return { + text, + calls, + warning: errors.some(message => message.includes("hasn't mounted yet")), + }; + } +}); + +function deferred() { + let resolve!: () => void; + const promise = new Promise(res => { + resolve = res; + }); + return { promise, resolve, done: false }; +} + +function treeText(renderer: TestRenderer.ReactTestRenderer | undefined) { + if (!renderer) return ''; + return collect(renderer.toJSON()); +} + +function collect(node: unknown): string { + if (node == null) return ''; + if (typeof node === 'string') return node; + if (Array.isArray(node)) return node.map(collect).join(''); + if (typeof node === 'object' && 'children' in node) + return collect((node as { children?: unknown }).children); + return ''; +} + +async function waitForText(read: () => string, ms: number) { + const start = Date.now(); + let text = ''; + while (Date.now() - start < ms) { + text = read(); + if (text.includes('value')) return text; + await new Promise(resolve => setTimeout(resolve, 20)); + } + return text; +} diff --git a/packages/react/src/components/__tests__/useSuspense-precommit.web.tsx b/packages/react/src/components/__tests__/useSuspense-precommit.web.tsx new file mode 100644 index 000000000000..ff5e9f80e21f --- /dev/null +++ b/packages/react/src/components/__tests__/useSuspense-precommit.web.tsx @@ -0,0 +1,157 @@ +import type { Manager, Middleware } from '@data-client/core'; +import { Endpoint } from '@data-client/endpoint'; +import React, { Suspense, use, useState } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; + +import { DataProvider, useSuspense } from '../..'; +import { getDefaultManagers } from '../getDefaultManagers'; + +/** + * The response microtask has to run before DataProvider commits. + * `renderDataHook` / `act` commit the provider first, so they hide this race. + */ +describe('useSuspense already-resolved endpoint before commit', () => { + let container: HTMLDivElement; + let root: Root | undefined; + let prevActEnv: boolean | undefined; + let errors: string[]; + let errorSpy: jest.SpyInstance; + + beforeEach(() => { + container = document.createElement('div'); + document.body.appendChild(container); + prevActEnv = (globalThis as any).IS_REACT_ACT_ENVIRONMENT; + (globalThis as any).IS_REACT_ACT_ENVIRONMENT = false; + errors = []; + errorSpy = jest.spyOn(console, 'error').mockImplementation((...args) => { + errors.push(args.map(String).join(' ')); + }); + }); + + afterEach(() => { + root?.unmount(); + root = undefined; + (globalThis as any).IS_REACT_ACT_ENVIRONMENT = prevActEnv; + container.remove(); + errorSpy.mockRestore(); + }); + + function mount(element: React.ReactElement) { + root = createRoot(container); + // Async-route shape: the route evaluates inside a resolved thenable. + return Promise.resolve().then(() => { + root!.render(element); + }); + } + + test('shows the resolved value once the commit flushes', async () => { + const observed = await renderRace({ park: 'none' }); + expect(observed.text).toBe('value 5'); + expect(observed.calls).toBeLessThanOrEqual(3); + expect(observed.warning).toBe(false); + expect(observed.committedBeforeResolve).toBe(true); + }); + + test('a parked render with a fresh provider still resolves', async () => { + const observed = await renderRace({ park: 'outside' }); + expect(observed.text).toBe('value 5'); + expect(observed.calls).toBeLessThanOrEqual(5); + expect(observed.warning).toBe(false); + expect(observed.committedBeforeResolve).toBe(true); + }); + + async function renderRace({ park }: { park: 'none' | 'outside' }) { + const probe = new CommitProbe(); + let calls = 0; + const endpoint = new Endpoint( + () => { + calls += 1; + if (calls > 30) return new Promise(() => undefined); + return Promise.resolve(5); + }, + { dataExpiryLength: Infinity }, + ); + const gate = deferred(); + + function Reader() { + const value = useSuspense(endpoint); + return value {String(value)}; + } + function Park() { + if (!gate.done) use(gate.promise); + return null; + } + function Store({ children }: { children: React.ReactNode }) { + // Each provider fiber builds its own managers. A module-level array + // is a different race and is not what this test locks. + const [managers] = useState(() => [...getDefaultManagers(), probe]); + return ( + + {children} + + ); + } + + await mount( + <> + + fallback}> + + + + {park === 'outside' ? + + : null} + , + ); + + // Let the endpoint microtask land before the provider's commit when the + // tree is parked, and before we release the gate. + await new Promise(resolve => setTimeout(resolve, 30)); + if (park !== 'none') { + gate.done = true; + gate.resolve(); + } + const text = await waitForText(container, 800); + return { + text, + calls, + warning: errors.some(message => message.includes("hasn't mounted yet")), + committedBeforeResolve: probe.committedBeforeResolve, + }; + } +}); + +class CommitProbe implements Manager { + committedBeforeResolve = false; + cleanup() { + this.committedBeforeResolve = false; + } + + middleware: Middleware = controller => next => action => { + if (action.type !== 'rdc/setresponse') return next(action); + const before = controller.getState(); + return Promise.resolve(next(action)).then(() => { + this.committedBeforeResolve = controller.getState() !== before; + }); + }; +} + +function deferred() { + let resolve!: () => void; + const promise = new Promise(res => { + resolve = res; + }); + return { promise, resolve, done: false }; +} + +async function waitForText(container: HTMLElement, ms: number) { + const start = Date.now(); + let text = container.textContent ?? ''; + while (Date.now() - start < ms) { + text = container.textContent ?? ''; + if (text.includes('value')) return text; + await new Promise(resolve => setTimeout(resolve, 20)); + } + return text; +} diff --git a/packages/use-enhanced-reducer/src/__tests__/precommit-dispatch.tsx b/packages/use-enhanced-reducer/src/__tests__/precommit-dispatch.tsx new file mode 100644 index 000000000000..bf7ba1934c3d --- /dev/null +++ b/packages/use-enhanced-reducer/src/__tests__/precommit-dispatch.tsx @@ -0,0 +1,109 @@ +import { useRef } from 'react'; + +import { renderHook, act } from '../../../test'; +import { MiddlewareAPI } from '../types'; +import useEnhancedReducer from '../useEnhancedReducer'; + +describe('useEnhancedReducer pre-commit dispatch', () => { + function ignoreError(e: Event) { + e.preventDefault(); + } + beforeEach(() => { + if (typeof addEventListener === 'function') + addEventListener('error', ignoreError); + }); + afterEach(() => { + if (typeof removeEventListener === 'function') + removeEventListener('error', ignoreError); + }); + + test('replays actions from the first render in order after commit', async () => { + const reduced: string[] = []; + const resolvedWith: string[][] = []; + const reducer = (state: string[], action: { type: string }) => { + reduced.push(action.type); + return [...state, action.type]; + }; + + const { result } = renderHook(() => { + const started = useRef(false); + const tuple = useEnhancedReducer(reducer, [] as string[], []); + if (!started.current) { + started.current = true; + const first = tuple[1]({ type: 'a' }); + const second = tuple[1]({ type: 'b' }); + expect(second).toBe(first); + void first.then(() => { + resolvedWith.push(tuple[2]()); + }); + } + return tuple; + }); + + await act(async () => { + await Promise.resolve(); + }); + + expect(reduced).toEqual(['a', 'b']); + expect(result.current[0]).toEqual(['a', 'b']); + expect(resolvedWith).toEqual([['a', 'b']]); + }); + + test('resolves the dispatch promise only after the replayed state commits', async () => { + const log: string[] = []; + const reducer = (state: number, action: { type: string }) => { + log.push(`reduce:${action.type}`); + return state + 1; + }; + + const { result } = renderHook(() => { + const started = useRef(false); + const [state, dispatch, getState] = useEnhancedReducer(reducer, 0, []); + if (!started.current) { + started.current = true; + void dispatch({ type: 'inc' }).then(() => { + log.push(`resolved:${getState()}`); + }); + } + return state; + }); + + await act(async () => { + await Promise.resolve(); + }); + + expect(result.current).toBe(1); + expect(log).toEqual(['reduce:inc', 'resolved:1']); + }); + + test('ignores dispatch after unmount', async () => { + const info = jest + .spyOn(console, 'info') + .mockImplementation(() => undefined); + const reducer = jest.fn((state: number) => state + 1); + let injected: MiddlewareAPI['dispatch'] = () => Promise.resolve(); + const capturing = ({ dispatch }: MiddlewareAPI) => { + injected = dispatch; + return (next: (action: any) => any) => (action: any) => next(action); + }; + try { + const { unmount } = renderHook(() => + useEnhancedReducer(reducer, 0, [capturing]), + ); + unmount(); + let resolved = false; + await act(async () => { + await injected({ type: 'late' }).then(() => { + resolved = true; + }); + }); + expect(resolved).toBe(true); + expect(reducer).not.toHaveBeenCalled(); + expect(info).toHaveBeenCalledWith( + 'Action dispatched after unmount. This will be ignored.', + ); + } finally { + info.mockRestore(); + } + }); +}); diff --git a/packages/use-enhanced-reducer/src/usePromisifiedDispatch.ts b/packages/use-enhanced-reducer/src/usePromisifiedDispatch.ts index 75424c1c8827..bea2975f2800 100644 --- a/packages/use-enhanced-reducer/src/usePromisifiedDispatch.ts +++ b/packages/use-enhanced-reducer/src/usePromisifiedDispatch.ts @@ -1,5 +1,5 @@ 'use client'; -import React, { useRef, useCallback, useEffect } from 'react'; +import React, { useRef, useCallback, useEffect, useLayoutEffect } from 'react'; import type { ReducerAction } from './ReducerAction.js'; @@ -10,7 +10,47 @@ export default function usePromisifiedDispatch< R extends React.Reducer, >(dispatch: React.Dispatch>, state: React.ReducerState) { const dispatchPromiseRef = useRef(null); + // Actions that arrive before the first commit. null once that commit's + // layout effect has run, which is also how this stays distinct from the + // post-unmount no-op installed by useEnhancedReducer. + const preCommitQueueRef = useRef[] | null>([]); + // Mount snapshot the flushed actions must not resolve against. The passive + // effect below runs for that snapshot too, and resolving there lets + // listeners read state before the reducer applies the response. + const awaitCommitFromRef = useRef | undefined>( + undefined, + ); + + useLayoutEffect(() => { + const queued = preCommitQueueRef.current; + if (queued === null) return; + preCommitQueueRef.current = null; + if (queued.length === 0) return; + awaitCommitFromRef.current = state; + for (let i = 0; i < queued.length; i++) { + dispatch(queued[i]); + } + // `state` is the snapshot these actions must commit past. Re-running this + // when it changes would flush an already-open gate. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [dispatch]); + useEffect(() => { + if ( + awaitCommitFromRef.current !== undefined && + state === awaitCommitFromRef.current + ) { + return; + } + awaitCommitFromRef.current = undefined; + // Layout effect has not opened the gate yet (SSR, or a host that runs + // passive effects first). Keep the commit promise pending. + if ( + preCommitQueueRef.current !== null && + preCommitQueueRef.current.length > 0 + ) { + return; + } if (dispatchPromiseRef.current) { dispatchPromiseRef.current.resolve(); dispatchPromiseRef.current = null; @@ -26,6 +66,11 @@ export default function usePromisifiedDispatch< // however that can also make the ref clear, so we need to make sure we have to promise before // dispatching so we can return it even if the ref changes. const promise = dispatchPromiseRef.current.promise; + const queued = preCommitQueueRef.current; + if (queued !== null) { + queued.push(action); + return promise; + } dispatch(action); return promise; },