diff --git a/.api-reports/api-report-react_internal.md b/.api-reports/api-report-react_internal.md index d81159f41cd..86141628ec6 100644 --- a/.api-reports/api-report-react_internal.md +++ b/.api-reports/api-report-react_internal.md @@ -1731,6 +1731,8 @@ interface SubscriptionOptions { class SuspenseCache { constructor(options?: SuspenseCacheOptions); // (undocumented) + add(cacheKey: CacheKey, queryRef: InternalQueryReference): void; + // (undocumented) getQueryRef(cacheKey: CacheKey, createObservable: () => ObservableQuery): InternalQueryReference; } diff --git a/.changeset/fuzzy-grapes-pump.md b/.changeset/fuzzy-grapes-pump.md new file mode 100644 index 00000000000..ec438245888 --- /dev/null +++ b/.changeset/fuzzy-grapes-pump.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where rerendering `useBackgroundQuery` after the `queryRef` had been disposed, either via the auto dispose timeout or by unmounting `useReadQuery`, would cause the `queryRef` to be recreated potentially resulting in another network request. diff --git a/.changeset/lovely-mayflies-grab.md b/.changeset/lovely-mayflies-grab.md new file mode 100644 index 00000000000..f0ebb865afa --- /dev/null +++ b/.changeset/lovely-mayflies-grab.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Allow queryRefs to be disposed of synchronously when a suspense hook unmounts. This prevents some situations where using a suspense hook with the same query/variables as the disposed queryRef accidentally used the disposed queryRef rather than creating a new instance. diff --git a/.size-limits.json b/.size-limits.json index f964b98787f..69a2f58b769 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39325, + "dist/apollo-client.min.cjs": 39368, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32634 } diff --git a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx index 198e3601aa3..11acab50152 100644 --- a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx +++ b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx @@ -410,7 +410,10 @@ it("auto resubscribes when mounting useReadQuery after naturally disposed by use await wait(0); expect(client.getObservableQueries().size).toBe(0); - expect(client).not.toHaveSuspenseCacheEntryUsing(query); + // We retain the cache entry in useBackgroundQuery to avoid recreating the + // queryRef if useBackgroundQuery rerenders before useReadQuery is mounted + // again. + expect(client).toHaveSuspenseCacheEntryUsing(query); await act(() => user.click(toggleButton)); @@ -447,6 +450,92 @@ it("auto resubscribes when mounting useReadQuery after naturally disposed by use await expect(Profiler).not.toRerender({ timeout: 50 }); }); +it("does not recreate queryRef and execute a network request when rerendering useBackgroundQuery after queryRef is disposed", async () => { + const { query } = setupSimpleCase(); + const user = userEvent.setup(); + let fetchCount = 0; + const client = new ApolloClient({ + link: new ApolloLink(() => { + fetchCount++; + + return new Observable((observer) => { + setTimeout(() => { + observer.next({ data: { greeting: "Hello" } }); + observer.complete(); + }, 20); + }); + }), + cache: new InMemoryCache(), + }); + + const Profiler = createDefaultProfiler(); + const { SuspenseFallback, ReadQueryHook } = + createDefaultTrackedComponents(Profiler); + + function App() { + useTrackRenders(); + const [show, setShow] = React.useState(true); + // Use a fetchPolicy of no-cache to ensure we can more easily track if + // another network request was made + const [queryRef] = useBackgroundQuery(query, { fetchPolicy: "no-cache" }); + + return ( + <> + + }> + {show && } + + + ); + } + + const { rerender } = renderWithClient(, { client, wrapper: Profiler }); + + const toggleButton = screen.getByText("Toggle"); + + { + const { renderedComponents } = await Profiler.takeRender(); + + expect(renderedComponents).toStrictEqual([App, SuspenseFallback]); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.result).toEqual({ + data: { greeting: "Hello" }, + error: undefined, + networkStatus: NetworkStatus.ready, + }); + } + + await act(() => user.click(toggleButton)); + await Profiler.takeRender(); + await wait(0); + + rerender(); + await Profiler.takeRender(); + + expect(fetchCount).toBe(1); + + await act(() => user.click(toggleButton)); + + { + const { snapshot, renderedComponents } = await Profiler.takeRender(); + + expect(renderedComponents).toStrictEqual([App, ReadQueryHook]); + expect(snapshot.result).toEqual({ + data: { greeting: "Hello" }, + error: undefined, + networkStatus: NetworkStatus.ready, + }); + } + + expect(fetchCount).toBe(1); + + await expect(Profiler).not.toRerender({ timeout: 50 }); +}); + it("disposes of the queryRef when unmounting before it is used by useReadQuery", async () => { const { query, mocks } = setupSimpleCase(); const client = new ApolloClient({ @@ -3672,6 +3761,79 @@ it('does not suspend deferred queries with partial data in the cache and using a await expect(Profiler).not.toRerender({ timeout: 50 }); }); +it.each([ + "cache-first", + "network-only", + "cache-and-network", +])( + 'responds to cache updates in strict mode while using a "%s" fetch policy', + async (fetchPolicy) => { + const { query, mocks } = setupSimpleCase(); + + const client = new ApolloClient({ + cache: new InMemoryCache(), + link: new MockLink(mocks), + }); + + const Profiler = createDefaultProfiler(); + const { SuspenseFallback, ReadQueryHook } = + createDefaultTrackedComponents(Profiler); + + function App() { + useTrackRenders(); + const [queryRef] = useBackgroundQuery(query, { fetchPolicy }); + + return ( + }> + + + ); + } + + renderWithClient(, { + client, + wrapper: ({ children }) => ( + + {children} + + ), + }); + + { + const { renderedComponents } = await Profiler.takeRender(); + + expect(renderedComponents).toStrictEqual([App, SuspenseFallback]); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.result).toEqual({ + data: { greeting: "Hello" }, + error: undefined, + networkStatus: NetworkStatus.ready, + }); + } + + client.writeQuery({ + query, + data: { greeting: "Updated hello" }, + }); + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.result).toEqual({ + data: { greeting: "Updated hello" }, + error: undefined, + networkStatus: NetworkStatus.ready, + }); + } + + await expect(Profiler).not.toRerender({ timeout: 50 }); + } +); + describe("refetch", () => { it("re-suspends when calling `refetch`", async () => { const { query, mocks: defaultMocks } = setupVariablesCase(); diff --git a/src/react/hooks/useBackgroundQuery.ts b/src/react/hooks/useBackgroundQuery.ts index ba5f8c19221..0b060b2476f 100644 --- a/src/react/hooks/useBackgroundQuery.ts +++ b/src/react/hooks/useBackgroundQuery.ts @@ -239,6 +239,20 @@ function _useBackgroundQuery< updateWrappedQueryRef(wrappedQueryRef, promise); } + // Handle strict mode where the query ref might be disposed when useEffect + // runs twice. We add the queryRef back in the suspense cache so that the next + // render will reuse this queryRef rather than initializing a new instance. + // This also prevents issues where rerendering useBackgroundQuery after the + // queryRef has been disposed, either automatically or by unmounting + // useReadQuery will ensure the same queryRef is maintained. + React.useEffect(() => { + if (queryRef.disposed) { + suspenseCache.add(cacheKey, queryRef); + } + // Omitting the deps is intentional. This avoids stale closures and the + // conditional ensures we aren't running the logic on each render. + }); + const fetchMore: FetchMoreFunction = React.useCallback( (options) => { const promise = queryRef.fetchMore(options as FetchMoreQueryOptions); diff --git a/src/react/hooks/useReadQuery.ts b/src/react/hooks/useReadQuery.ts index 9e8e4621a83..6c1ff70d4e8 100644 --- a/src/react/hooks/useReadQuery.ts +++ b/src/react/hooks/useReadQuery.ts @@ -64,7 +64,17 @@ function _useReadQuery( updateWrappedQueryRef(queryRef, internalQueryRef.promise); } - React.useEffect(() => internalQueryRef.retain(), [internalQueryRef]); + React.useEffect(() => { + // It may seem odd that we are trying to reinitialize the queryRef even + // though we reinitialize in render above, but this is necessary to + // handle strict mode where this useEffect will be run twice resulting in a + // disposed queryRef before the next render. + if (internalQueryRef.disposed) { + internalQueryRef.reinitialize(); + } + + return internalQueryRef.retain(); + }, [internalQueryRef]); const promise = useSyncExternalStore( React.useCallback( diff --git a/src/react/hooks/useSuspenseQuery.ts b/src/react/hooks/useSuspenseQuery.ts index 77159eb31f2..0459aba5087 100644 --- a/src/react/hooks/useSuspenseQuery.ts +++ b/src/react/hooks/useSuspenseQuery.ts @@ -239,6 +239,34 @@ function _useSuspenseQuery< }; }, [queryRef]); + // This effect handles the case where strict mode causes the queryRef to get + // disposed early. Previously this was done by using a `setTimeout` inside the + // dispose function, but this could cause some issues in cases where someone + // might expect the queryRef to be disposed immediately. For example, when + // using the same client instance across multiple tests in a test suite, the + // `setTimeout` has the possibility of retaining the suspense cache entry for + // too long, which means that two tests might accidentally share the same + // `queryRef` instance. By immediately disposing, we can avoid this situation. + // + // Instead we can leverage the work done to allow the queryRef to "resume" + // after it has been disposed without executing an additional network request. + // This is done by calling the `initialize` function below. + React.useEffect(() => { + if (queryRef.disposed) { + // Calling the `dispose` function removes it from the suspense cache, so + // when the component rerenders, it instantiates a fresh query ref. + // We address this by adding the queryRef back to the suspense cache + // so that the lookup on the next render uses the existing queryRef rather + // than instantiating a new one. + suspenseCache.add(cacheKey, queryRef); + queryRef.reinitialize(); + } + // We can omit the deps here to get a fresh closure each render since the + // conditional will prevent the logic from running in most cases. This + // should also be a touch faster since it should be faster to check the `if` + // statement than for React to compare deps on this effect. + }); + const skipResult = React.useMemo(() => { const error = toApolloError(queryRef.result); diff --git a/src/react/internal/cache/QueryReference.ts b/src/react/internal/cache/QueryReference.ts index c40a155a28f..7645ef0d6f0 100644 --- a/src/react/internal/cache/QueryReference.ts +++ b/src/react/internal/cache/QueryReference.ts @@ -242,12 +242,9 @@ export class InternalQueryReference { disposed = true; this.references--; - // Wait before fully disposing in case the app is running in strict mode. - setTimeout(() => { - if (!this.references) { - this.dispose(); - } - }); + if (!this.references) { + this.dispose(); + } }; } diff --git a/src/react/internal/cache/SuspenseCache.ts b/src/react/internal/cache/SuspenseCache.ts index d66eb905a5a..8b1eba321b5 100644 --- a/src/react/internal/cache/SuspenseCache.ts +++ b/src/react/internal/cache/SuspenseCache.ts @@ -47,4 +47,9 @@ export class SuspenseCache { return ref.current; } + + add(cacheKey: CacheKey, queryRef: InternalQueryReference) { + const ref = this.queryRefs.lookupArray(cacheKey); + ref.current = queryRef; + } }