From 987ff6dbb0c64d0ea128c054f26889889ccee6dd Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Fri, 17 May 2024 16:21:21 +0200 Subject: [PATCH 01/13] Revert "Merge pull request #9707 from kazekyo/fix_usesubscription_in_strict_mode" This reverts commit 4571e1ad0c2d2b9d9bf072f0f004b4487d55bc76, reversing changes made to 5be85a0ee22a5e4812d1a3da2b036b1d1580b4a5. --- src/react/hooks/useSubscription.ts | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 366ebfe97f4..e173eecbad1 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -149,13 +149,6 @@ export function useSubscription< }); }); - const canResetObservableRef = React.useRef(false); - React.useEffect(() => { - return () => { - canResetObservableRef.current = true; - }; - }, []); - const ref = React.useRef({ client, subscription, options }); React.useEffect(() => { let shouldResubscribe = options?.shouldResubscribe; @@ -164,10 +157,7 @@ export function useSubscription< } if (options?.skip) { - if ( - !options?.skip !== !ref.current.options?.skip || - canResetObservableRef.current - ) { + if (!options?.skip !== !ref.current.options?.skip) { setResult({ loading: false, data: void 0, @@ -175,16 +165,14 @@ export function useSubscription< variables: options?.variables, }); setObservable(null); - canResetObservableRef.current = false; } } else if ( - (shouldResubscribe !== false && - (client !== ref.current.client || - subscription !== ref.current.subscription || - options?.fetchPolicy !== ref.current.options?.fetchPolicy || - !options?.skip !== !ref.current.options?.skip || - !equal(options?.variables, ref.current.options?.variables))) || - canResetObservableRef.current + shouldResubscribe !== false && + (client !== ref.current.client || + subscription !== ref.current.subscription || + options?.fetchPolicy !== ref.current.options?.fetchPolicy || + !options?.skip !== !ref.current.options?.skip || + !equal(options?.variables, ref.current.options?.variables)) ) { setResult({ loading: true, @@ -200,11 +188,10 @@ export function useSubscription< context: options?.context, }) ); - canResetObservableRef.current = false; } Object.assign(ref.current, { client, subscription, options }); - }, [client, subscription, options, canResetObservableRef.current]); + }, [client, subscription, options]); React.useEffect(() => { if (!observable) { From 03d89ea0833602d7901cfb36df9f9b2c987dc46d Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 11:36:33 +0200 Subject: [PATCH 02/13] essentially rewrite useSubscription --- src/react/hooks/useSubscription.ts | 316 ++++++++++++++++++----------- 1 file changed, 192 insertions(+), 124 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index e173eecbad1..5f626f089c4 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -10,8 +10,18 @@ import type { SubscriptionHookOptions, SubscriptionResult, } from "../types/types.js"; -import type { OperationVariables } from "../../core/index.js"; +import type { + ApolloClient, + DefaultContext, + FetchPolicy, + OperationVariables, +} from "../../core/index.js"; import { useApolloClient } from "./useApolloClient.js"; +import { useDeepMemo } from "./internal/useDeepMemo.js"; +import { useSyncExternalStore } from "./useSyncExternalStore.js"; + +const emptyObject = Object.freeze({}); + /** * > Refer to the [Subscriptions](https://www.apollographql.com/docs/react/data/subscriptions/) section for a more in-depth overview of `useSubscription`. * @@ -102,24 +112,19 @@ export function useSubscription< TVariables extends OperationVariables = OperationVariables, >( subscription: DocumentNode | TypedDocumentNode, - options?: SubscriptionHookOptions, NoInfer> + options: SubscriptionHookOptions< + NoInfer, + NoInfer + > = emptyObject ) { const hasIssuedDeprecationWarningRef = React.useRef(false); - const client = useApolloClient(options?.client); + const client = useApolloClient(options.client); verifyDocumentType(subscription, DocumentType.Subscription); - const [result, setResult] = React.useState< - SubscriptionResult - >({ - loading: !options?.skip, - error: void 0, - data: void 0, - variables: options?.variables, - }); if (!hasIssuedDeprecationWarningRef.current) { hasIssuedDeprecationWarningRef.current = true; - if (options?.onSubscriptionData) { + if (options.onSubscriptionData) { invariant.warn( options.onData ? "'useSubscription' supports only the 'onSubscriptionData' or 'onData' option, but not both. Only the 'onData' option will be used." @@ -127,7 +132,7 @@ export function useSubscription< ); } - if (options?.onSubscriptionComplete) { + if (options.onSubscriptionComplete) { invariant.warn( options.onComplete ? "'useSubscription' supports only the 'onSubscriptionComplete' or 'onComplete' option, but not both. Only the 'onComplete' option will be used." @@ -136,129 +141,192 @@ export function useSubscription< } } - const [observable, setObservable] = React.useState(() => { - if (options?.skip) { - return null; - } - - return client.subscribe({ - query: subscription, - variables: options?.variables, - fetchPolicy: options?.fetchPolicy, - context: options?.context, - }); - }); + let { skip, fetchPolicy, variables, shouldResubscribe, context } = options; + variables = useDeepMemo(() => variables, [variables]); + if (typeof shouldResubscribe === "function") { + shouldResubscribe = !!shouldResubscribe(options!); + } - const ref = React.useRef({ client, subscription, options }); + const contextRef = React.useRef(context); React.useEffect(() => { - let shouldResubscribe = options?.shouldResubscribe; - if (typeof shouldResubscribe === "function") { - shouldResubscribe = !!shouldResubscribe(options!); - } + contextRef.current = context; + }, [context]); - if (options?.skip) { - if (!options?.skip !== !ref.current.options?.skip) { - setResult({ - loading: false, - data: void 0, - error: void 0, - variables: options?.variables, - }); - setObservable(null); + const [observable, setObservable] = React.useState(() => + options.skip ? null : ( + createSubscription( + client, + subscription, + variables, + fetchPolicy, + contextRef.current + ) + ) + ); + + React.useEffect(() => { + function resubscriptionEffect() { + if (skip) { + if (observable) { + setObservable(null); + } + } else if ( + !observable || + (shouldResubscribe !== false && + (client !== observable.__.client || + subscription !== observable.__.query || + !equal(variables, observable.__.variables) || + fetchPolicy !== observable.__.fetchPolicy)) + ) { + setObservable( + createSubscription( + client, + subscription, + variables, + fetchPolicy, + contextRef.current + ) + ); } - } else if ( - shouldResubscribe !== false && - (client !== ref.current.client || - subscription !== ref.current.subscription || - options?.fetchPolicy !== ref.current.options?.fetchPolicy || - !options?.skip !== !ref.current.options?.skip || - !equal(options?.variables, ref.current.options?.variables)) - ) { - setResult({ - loading: true, - data: void 0, - error: void 0, - variables: options?.variables, - }); - setObservable( - client.subscribe({ - query: subscription, - variables: options?.variables, - fetchPolicy: options?.fetchPolicy, - context: options?.context, - }) - ); } + const id = setTimeout(resubscriptionEffect); + return () => clearTimeout(id); + }, [ + client, + fetchPolicy, + observable, + shouldResubscribe, + skip, + subscription, + variables, + ]); - Object.assign(ref.current, { client, subscription, options }); - }, [client, subscription, options]); - + const optionsRef = React.useRef(options); React.useEffect(() => { - if (!observable) { - return; - } + optionsRef.current = options; + }); + + const fallbackResult = React.useMemo>( + () => ({ + loading: !skip, + error: void 0, + data: void 0, + variables: variables, + }), + [skip, variables] + ); - let subscriptionStopped = false; - const subscription = observable.subscribe({ - next(fetchResult) { - if (subscriptionStopped) { - return; + return useSyncExternalStore>( + React.useCallback( + (update) => { + if (!observable) { + return () => {}; } - const result = { - loading: false, - // TODO: fetchResult.data can be null but SubscriptionResult.data - // expects TData | undefined only - data: fetchResult.data!, - error: void 0, - variables: options?.variables, - }; - setResult(result); + let subscriptionStopped = false; + const variables = observable.__.variables; + const client = observable.__.client; + const subscription = observable.subscribe({ + next(fetchResult) { + if (subscriptionStopped) { + return; + } - if (ref.current.options?.onData) { - ref.current.options.onData({ - client, - data: result, - }); - } else if (ref.current.options?.onSubscriptionData) { - ref.current.options.onSubscriptionData({ - client, - subscriptionData: result, - }); - } - }, - error(error) { - if (!subscriptionStopped) { - setResult({ - loading: false, - data: void 0, - error, - variables: options?.variables, + const result = { + loading: false, + // TODO: fetchResult.data can be null but SubscriptionResult.data + // expects TData | undefined only + data: fetchResult.data!, + error: void 0, + variables, + }; + observable.__.result = result; + update(); + + if (optionsRef.current.onData) { + optionsRef.current.onData({ + client, + data: result, + }); + } else if (optionsRef.current.onSubscriptionData) { + optionsRef.current.onSubscriptionData({ + client, + subscriptionData: result, + }); + } + }, + error(error) { + if (!subscriptionStopped) { + observable.__.result = { + loading: false, + data: void 0, + error, + variables, + }; + update(); + optionsRef.current.onError?.(error); + } + }, + complete() { + if (!subscriptionStopped) { + if (optionsRef.current.onComplete) { + optionsRef.current.onComplete(); + } else if (optionsRef.current.onSubscriptionComplete) { + optionsRef.current.onSubscriptionComplete(); + } + } + }, + }); + + return () => { + // immediately stop receiving subscription values, but do not unsubscribe + // until after a short delay in case another useSubscription hook is + // reusing the same underlying observable and is about to subscribe + subscriptionStopped = true; + setTimeout(() => { + subscription.unsubscribe(); }); - ref.current.options?.onError?.(error); - } - }, - complete() { - if (!subscriptionStopped) { - if (ref.current.options?.onComplete) { - ref.current.options.onComplete(); - } else if (ref.current.options?.onSubscriptionComplete) { - ref.current.options.onSubscriptionComplete(); - } - } + }; }, - }); - - return () => { - // immediately stop receiving subscription values, but do not unsubscribe - // until after a short delay in case another useSubscription hook is - // reusing the same underlying observable and is about to subscribe - subscriptionStopped = true; - setTimeout(() => { - subscription.unsubscribe(); - }); - }; - }, [observable]); + [observable] + ), + React.useCallback( + () => (observable && !skip ? observable.__.result : fallbackResult), + [observable, fallbackResult, skip] + ) + ); +} - return result; +function createSubscription< + TData = any, + TVariables extends OperationVariables = OperationVariables, +>( + client: ApolloClient, + query: TypedDocumentNode, + variables?: TVariables, + fetchPolicy?: FetchPolicy, + context?: DefaultContext +) { + return Object.assign( + client.subscribe({ + query, + variables, + fetchPolicy, + context, + }), + { + __: { + variables, + client, + query, + fetchPolicy, + result: { + loading: true, + data: void 0, + error: void 0, + variables, + } as SubscriptionResult, + }, + } + ); } From 06db150fefc3d14b66fb13c8af6f694edef84fc8 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 13:32:56 +0200 Subject: [PATCH 03/13] use `setResult` update method --- src/react/hooks/useSubscription.ts | 42 ++++++++++++++++-------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 5f626f089c4..f64ffa87976 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -240,7 +240,7 @@ export function useSubscription< error: void 0, variables, }; - observable.__.result = result; + observable.__.setResult(result); update(); if (optionsRef.current.onData) { @@ -257,12 +257,12 @@ export function useSubscription< }, error(error) { if (!subscriptionStopped) { - observable.__.result = { + observable.__.setResult({ loading: false, data: void 0, error, variables, - }; + }); update(); optionsRef.current.onError?.(error); } @@ -290,10 +290,7 @@ export function useSubscription< }, [observable] ), - React.useCallback( - () => (observable && !skip ? observable.__.result : fallbackResult), - [observable, fallbackResult, skip] - ) + () => (observable && !skip ? observable.__.result : fallbackResult) ); } @@ -307,6 +304,21 @@ function createSubscription< fetchPolicy?: FetchPolicy, context?: DefaultContext ) { + const __ = { + variables, + client, + query, + fetchPolicy, + result: { + loading: true, + data: void 0, + error: void 0, + variables, + } as SubscriptionResult, + setResult(result: SubscriptionResult) { + __.result = result; + }, + }; return Object.assign( client.subscribe({ query, @@ -315,18 +327,10 @@ function createSubscription< context, }), { - __: { - variables, - client, - query, - fetchPolicy, - result: { - loading: true, - data: void 0, - error: void 0, - variables, - } as SubscriptionResult, - }, + /** + * A tracking object to store details about the observable and the latest result of the subscription. + */ + __, } ); } From 71dc7aa0e6bbab3fb8ba54b64a99e53197f733e9 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 15:05:42 +0200 Subject: [PATCH 04/13] adjust tests --- .size-limits.json | 2 +- .../__tests__/client/Subscription.test.tsx | 388 +++++++++--------- .../subscriptions/subscriptions.test.tsx | 20 +- .../__tests__/mockSubscriptionLink.test.tsx | 4 +- 4 files changed, 208 insertions(+), 206 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index baaaf675f5a..76b4c1ee94e 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39573, + "dist/apollo-client.min.cjs": 39592, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821 } diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index 4584913a30d..bfef7f8cc4b 100644 --- a/src/react/components/__tests__/client/Subscription.test.tsx +++ b/src/react/components/__tests__/client/Subscription.test.tsx @@ -5,10 +5,10 @@ import { render, waitFor } from "@testing-library/react"; import { ApolloClient } from "../../../../core"; import { InMemoryCache as Cache } from "../../../../cache"; import { ApolloProvider } from "../../../context"; -import { ApolloLink, Operation } from "../../../../link/core"; +import { ApolloLink, DocumentNode, Operation } from "../../../../link/core"; import { itAsync, MockSubscriptionLink } from "../../../../testing"; import { Subscription } from "../../Subscription"; -import { spyOnConsole } from "../../../../testing/internal"; +import { profile, spyOnConsole } from "../../../../testing/internal"; const results = [ "Luke Skywalker", @@ -422,77 +422,84 @@ describe("should update", () => { cache: new Cache({ addTypename: false }), }); - let count = 0; - let testFailures: any[] = []; + function Container() { + return ( + + {(r: any) => { + ProfiledContainer.replaceSnapshot(r); + return null; + }} + + ); + } + const ProfiledContainer = profile({ + Component: Container, + }); - class Component extends React.Component { - state = { - client: client, - }; + const { rerender } = render( + + + + ); - render() { - return ( - - - {(result: any) => { - const { loading, data } = result; - try { - switch (count++) { - case 0: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 1: - setTimeout(() => { - this.setState( - { - client: client2, - }, - () => { - link2.simulateResult(results[1]); - } - ); - }); - // fallthrough - case 2: - expect(loading).toBeFalsy(); - expect(data).toEqual(results[0].result.data); - break; - case 3: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 4: - expect(loading).toBeFalsy(); - expect(data).toEqual(results[1].result.data); - break; - default: - throw new Error("too many rerenders"); - } - } catch (error) { - testFailures.push(error); - } - return null; - }} - - - ); - } + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); } - render(); - link.simulateResult(results[0]); - await waitFor(() => { - if (testFailures.length > 0) { - throw testFailures[0]; - } - expect(count).toBe(5); - }); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(results[0].result.data); + } + + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); + + rerender( + + + + ); + + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(results[0].result.data); + } + + { + const { + snapshot, + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + console.log(snapshot); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + } + + link2.simulateResult(results[1]); + + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(results[1].result.data); + } + + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); }); - itAsync("if the query changes", (resolve, reject) => { + it("if the query changes", async () => { const subscriptionHero = gql` subscription HeroInfo { hero { @@ -524,72 +531,79 @@ describe("should update", () => { cache: new Cache({ addTypename: false }), }); - let count = 0; - - class Component extends React.Component { - state = { - subscription, - }; - - render() { - return ( - - {(result: any) => { - const { loading, data } = result; - try { - switch (count) { - case 0: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 1: - setTimeout(() => { - this.setState( - { - subscription: subscriptionHero, - }, - () => { - heroLink.simulateResult(heroResult); - } - ); - }); - // fallthrough - case 2: - expect(loading).toBeFalsy(); - expect(data).toEqual(results[0].result.data); - break; - case 3: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 4: - expect(loading).toBeFalsy(); - expect(data).toEqual(heroResult.result.data); - break; - } - } catch (error) { - reject(error); - } - count++; - return null; - }} - - ); - } + function Container({ subscription }: { subscription: DocumentNode }) { + return ( + + {(r: any) => { + ProfiledContainer.replaceSnapshot(r); + return null; + }} + + ); } + const ProfiledContainer = profile({ + Component: Container, + }); - render( - - - + const { rerender } = render( + , + { + wrapper: ({ children }) => ( + {children} + ), + } ); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + } userLink.simulateResult(results[0]); - waitFor(() => expect(count).toBe(5)).then(resolve, reject); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(results[0].result.data); + } + + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); + + rerender(); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(results[0].result.data); + } + + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + } + + heroLink.simulateResult(heroResult); + + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(heroResult.result.data); + } + + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); }); - itAsync("if the variables change", (resolve, reject) => { + it("if the variables change", async () => { const subscriptionWithVariables = gql` subscription UserInfo($name: String) { user(name: $name) { @@ -620,75 +634,79 @@ describe("should update", () => { cache, }); - let count = 0; + function Container({ variables }: { variables: any }) { + return ( + + {(r: any) => { + ProfiledContainer.replaceSnapshot(r); + return null; + }} + + ); + } + const ProfiledContainer = profile({ + Component: Container, + }); - class Component extends React.Component { - state = { - variables: variablesLuke, - }; + const { rerender } = render( + , + { + wrapper: ({ children }) => ( + {children} + ), + } + ); - render() { - return ( - - {(result: any) => { - const { loading, data } = result; - try { - switch (count) { - case 0: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 1: - setTimeout(() => { - this.setState( - { - variables: variablesHan, - }, - () => { - mockLink.simulateResult({ - result: { data: dataHan }, - }); - } - ); - }); - // fallthrough - case 2: - expect(loading).toBeFalsy(); - expect(data).toEqual(dataLuke); - break; - case 3: - expect(loading).toBeTruthy(); - expect(data).toBeUndefined(); - break; - case 4: - expect(loading).toBeFalsy(); - expect(data).toEqual(dataHan); - break; - } - } catch (error) { - reject(error); - } + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + } + mockLink.simulateResult({ result: { data: dataLuke } }); - count++; - return null; - }} - - ); - } + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(dataLuke); } - render( - - - - ); + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); - mockLink.simulateResult({ result: { data: dataLuke } }); + rerender(); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(dataLuke); + } + + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + } + mockLink.simulateResult({ + result: { data: dataHan }, + }); + { + const { + snapshot: { loading, data }, + } = await ProfiledContainer.takeRender(); + expect(loading).toBeFalsy(); + expect(data).toEqual(dataHan); + } - waitFor(() => expect(count).toBe(5)).then(resolve, reject); + await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); }); }); diff --git a/src/react/hoc/__tests__/subscriptions/subscriptions.test.tsx b/src/react/hoc/__tests__/subscriptions/subscriptions.test.tsx index b81567ca0d5..ecb517fd14e 100644 --- a/src/react/hoc/__tests__/subscriptions/subscriptions.test.tsx +++ b/src/react/hoc/__tests__/subscriptions/subscriptions.test.tsx @@ -11,8 +11,6 @@ import { itAsync, MockSubscriptionLink } from "../../../../testing"; import { graphql } from "../../graphql"; import { ChildProps } from "../../types"; -const IS_REACT_18 = React.version.startsWith("18"); - describe("subscriptions", () => { let error: typeof console.error; @@ -301,29 +299,17 @@ describe("subscriptions", () => { if (count === 0) expect(user).toEqual(results[0].result.data.user); if (count === 1) { - if (IS_REACT_18) { - expect(user).toEqual(results[1].result.data.user); - } else { - expect(user).toEqual(results[0].result.data.user); - } + expect(user).toEqual(results[0].result.data.user); } if (count === 2) expect(user).toEqual(results[2].result.data.user); if (count === 3) expect(user).toEqual(results[2].result.data.user); if (count === 4) { - if (IS_REACT_18) { - expect(user).toEqual(results[2].result.data.user); - } else { - expect(user).toEqual(results3[2].result.data.user); - } + expect(user).toEqual(results3[2].result.data.user); } if (count === 5) { - if (IS_REACT_18) { - expect(user).toEqual(results3[3].result.data.user); - } else { - expect(user).toEqual(results3[2].result.data.user); - } + expect(user).toEqual(results3[2].result.data.user); resolve(); } } catch (e) { diff --git a/src/testing/react/__tests__/mockSubscriptionLink.test.tsx b/src/testing/react/__tests__/mockSubscriptionLink.test.tsx index 0515e45cb0a..31c3abe70f7 100644 --- a/src/testing/react/__tests__/mockSubscriptionLink.test.tsx +++ b/src/testing/react/__tests__/mockSubscriptionLink.test.tsx @@ -8,8 +8,6 @@ import { InMemoryCache as Cache } from "../../../cache"; import { ApolloProvider } from "../../../react/context"; import { useSubscription } from "../../../react/hooks"; -const IS_REACT_18 = React.version.startsWith("18"); - describe("mockSubscriptionLink", () => { it("should work with multiple subscribers to the same mock websocket", async () => { const subscription = gql` @@ -64,7 +62,7 @@ describe("mockSubscriptionLink", () => { ); - const numRenders = IS_REACT_18 ? 2 : results.length + 1; + const numRenders = results.length + 1; // automatic batching in React 18 means we only see 2 renders vs. 5 in v17 await waitFor( From ac0d8049c46b6ced271c039ee7c53e585ce0c977 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 16:26:24 +0200 Subject: [PATCH 05/13] change observable during render; lazy initialization of subscription --- .size-limits.json | 2 +- .../__tests__/client/Subscription.test.tsx | 23 ----- src/react/hooks/useSubscription.ts | 91 ++++++++----------- 3 files changed, 39 insertions(+), 77 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 76b4c1ee94e..39f479ac305 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39592, + "dist/apollo-client.min.cjs": 39585, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821 } diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index bfef7f8cc4b..262a26a714d 100644 --- a/src/react/components/__tests__/client/Subscription.test.tsx +++ b/src/react/components/__tests__/client/Subscription.test.tsx @@ -468,14 +468,6 @@ describe("should update", () => { ); - { - const { - snapshot: { loading, data }, - } = await ProfiledContainer.takeRender(); - expect(loading).toBeFalsy(); - expect(data).toEqual(results[0].result.data); - } - { const { snapshot, @@ -574,14 +566,6 @@ describe("should update", () => { await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); rerender(); - { - const { - snapshot: { loading, data }, - } = await ProfiledContainer.takeRender(); - expect(loading).toBeFalsy(); - expect(data).toEqual(results[0].result.data); - } - { const { snapshot: { loading, data }, @@ -680,13 +664,6 @@ describe("should update", () => { await expect(ProfiledContainer).not.toRerender({ timeout: 50 }); rerender(); - { - const { - snapshot: { loading, data }, - } = await ProfiledContainer.takeRender(); - expect(loading).toBeFalsy(); - expect(data).toEqual(dataLuke); - } { const { diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index f64ffa87976..afa1a7323ef 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -14,8 +14,10 @@ import type { ApolloClient, DefaultContext, FetchPolicy, + FetchResult, OperationVariables, } from "../../core/index.js"; +import { Observable } from "../../core/index.js"; import { useApolloClient } from "./useApolloClient.js"; import { useDeepMemo } from "./internal/useDeepMemo.js"; import { useSyncExternalStore } from "./useSyncExternalStore.js"; @@ -147,59 +149,34 @@ export function useSubscription< shouldResubscribe = !!shouldResubscribe(options!); } - const contextRef = React.useRef(context); - React.useEffect(() => { - contextRef.current = context; - }, [context]); - - const [observable, setObservable] = React.useState(() => + let [observable, setObservable] = React.useState(() => options.skip ? null : ( - createSubscription( - client, - subscription, - variables, - fetchPolicy, - contextRef.current - ) + createSubscription(client, subscription, variables, fetchPolicy, context) ) ); - React.useEffect(() => { - function resubscriptionEffect() { - if (skip) { - if (observable) { - setObservable(null); - } - } else if ( - !observable || - (shouldResubscribe !== false && - (client !== observable.__.client || - subscription !== observable.__.query || - !equal(variables, observable.__.variables) || - fetchPolicy !== observable.__.fetchPolicy)) - ) { - setObservable( - createSubscription( - client, - subscription, - variables, - fetchPolicy, - contextRef.current - ) - ); - } + if (skip) { + if (observable) { + setObservable((observable = null)); } - const id = setTimeout(resubscriptionEffect); - return () => clearTimeout(id); - }, [ - client, - fetchPolicy, - observable, - shouldResubscribe, - skip, - subscription, - variables, - ]); + } else if ( + !observable || + (shouldResubscribe !== false && + (client !== observable.__.client || + subscription !== observable.__.query || + !equal(variables, observable.__.variables) || + fetchPolicy !== observable.__.fetchPolicy)) + ) { + setObservable( + (observable = createSubscription( + client, + subscription, + variables, + fetchPolicy, + context + )) + ); + } const optionsRef = React.useRef(options); React.useEffect(() => { @@ -319,12 +296,20 @@ function createSubscription< __.result = result; }, }; + + let observable: Observable> | null = null; return Object.assign( - client.subscribe({ - query, - variables, - fetchPolicy, - context, + new Observable>((observer) => { + // lazily start the subscription when the first observer subscribes + // to get around strict mode + observable ||= client.subscribe({ + query, + variables, + fetchPolicy, + context, + }); + const sub = observable.subscribe(observer); + return () => sub.unsubscribe(); }), { /** From 4aa14cfe643356802f01b897130f3cd589c57e3a Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 16:50:43 +0200 Subject: [PATCH 06/13] no more need for stable options, performance optimization --- .size-limits.json | 2 +- src/react/hooks/useSubscription.ts | 22 ++++++++-------------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 39f479ac305..baaaf675f5a 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39585, + "dist/apollo-client.min.cjs": 39573, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821 } diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index afa1a7323ef..13cba3588c4 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -22,8 +22,6 @@ import { useApolloClient } from "./useApolloClient.js"; import { useDeepMemo } from "./internal/useDeepMemo.js"; import { useSyncExternalStore } from "./useSyncExternalStore.js"; -const emptyObject = Object.freeze({}); - /** * > Refer to the [Subscriptions](https://www.apollographql.com/docs/react/data/subscriptions/) section for a more in-depth overview of `useSubscription`. * @@ -114,10 +112,7 @@ export function useSubscription< TVariables extends OperationVariables = OperationVariables, >( subscription: DocumentNode | TypedDocumentNode, - options: SubscriptionHookOptions< - NoInfer, - NoInfer - > = emptyObject + options: SubscriptionHookOptions, NoInfer> = {} ) { const hasIssuedDeprecationWarningRef = React.useRef(false); const client = useApolloClient(options.client); @@ -145,9 +140,6 @@ export function useSubscription< let { skip, fetchPolicy, variables, shouldResubscribe, context } = options; variables = useDeepMemo(() => variables, [variables]); - if (typeof shouldResubscribe === "function") { - shouldResubscribe = !!shouldResubscribe(options!); - } let [observable, setObservable] = React.useState(() => options.skip ? null : ( @@ -161,11 +153,13 @@ export function useSubscription< } } else if ( !observable || - (shouldResubscribe !== false && - (client !== observable.__.client || - subscription !== observable.__.query || - !equal(variables, observable.__.variables) || - fetchPolicy !== observable.__.fetchPolicy)) + ((client !== observable.__.client || + subscription !== observable.__.query || + fetchPolicy !== observable.__.fetchPolicy || + !equal(variables, observable.__.variables)) && + (typeof shouldResubscribe === "function" ? + !!shouldResubscribe(options!) + : shouldResubscribe) !== false) ) { setObservable( (observable = createSubscription( From af260cff32329c91d81ba067192ac9e68deacd18 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 21 May 2024 16:55:36 +0200 Subject: [PATCH 07/13] changeset --- .changeset/little-suits-return.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/little-suits-return.md diff --git a/.changeset/little-suits-return.md b/.changeset/little-suits-return.md new file mode 100644 index 00000000000..9206b615470 --- /dev/null +++ b/.changeset/little-suits-return.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": minor +--- + +Reimplement `useSubscription` From e249bad49efd6cb920b01e9ca2ecb7609657ce61 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 22 May 2024 11:58:22 +0200 Subject: [PATCH 08/13] adjust documentation --- src/react/hooks/useSubscription.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 13cba3588c4..d66ffeaf7b8 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -45,11 +45,10 @@ import { useSyncExternalStore } from "./useSyncExternalStore.js"; * } * ``` * @remarks - * #### Subscriptions and React 18 Automatic Batching + * #### Consider using `onData` instead of `useEffect` * - * With React 18's [automatic batching](https://react.dev/blog/2022/03/29/react-v18#new-feature-automatic-batching), multiple state updates may be grouped into a single re-render for better performance. - * - * If your subscription API sends multiple messages at the same time or in very fast succession (within fractions of a millisecond), it is likely that only the last message received in that narrow time frame will result in a re-render. + * If you want to react to incoming data, please use the `onData` option instead of `useEffect`. State updates you make inside a `useEffect` hook might cause additional rerenders, and `useEffect` is mostly meant for side effects of rendering, not as an event handler. + * That's why we provide you with `onData` as an option to `useSubscription`. * * Consider the following component: * @@ -71,10 +70,6 @@ import { useSyncExternalStore } from "./useSyncExternalStore.js"; * } * ``` * - * If your subscription back-end emits two messages with the same timestamp, only the last message received by Apollo Client will be rendered. This is because React 18 will batch these two state updates into a single re-render. - * - * Since the component above is using `useEffect` to push `data` into a piece of local state on each `Subscriptions` re-render, the first message will never be added to the `accumulatedData` array since its render was skipped. - * * Instead of using `useEffect` here, we can re-write this component to use the `onData` callback function accepted in `useSubscription`'s `options` object: * * ```jsx From 297919f167b3062fe1d89d3218aa3c0705383075 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 25 Jun 2024 11:23:54 +0200 Subject: [PATCH 09/13] review feedback --- src/react/hooks/useSubscription.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index d66ffeaf7b8..aa389db2c8e 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -107,7 +107,10 @@ export function useSubscription< TVariables extends OperationVariables = OperationVariables, >( subscription: DocumentNode | TypedDocumentNode, - options: SubscriptionHookOptions, NoInfer> = {} + options: SubscriptionHookOptions< + NoInfer, + NoInfer + > = Object.create(null) ) { const hasIssuedDeprecationWarningRef = React.useRef(false); const client = useApolloClient(options.client); @@ -133,8 +136,8 @@ export function useSubscription< } } - let { skip, fetchPolicy, variables, shouldResubscribe, context } = options; - variables = useDeepMemo(() => variables, [variables]); + const { skip, fetchPolicy, shouldResubscribe, context } = options; + const variables = useDeepMemo(() => options.variables, [options.variables]); let [observable, setObservable] = React.useState(() => options.skip ? null : ( From 9f64d3f7fa3739150d9391d3520f5c840156a3a8 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 3 Jul 2024 12:17:23 +0200 Subject: [PATCH 10/13] Apply suggestions from code review Co-authored-by: Jerel Miller --- .changeset/little-suits-return.md | 2 +- src/react/components/__tests__/client/Subscription.test.tsx | 1 - src/react/hooks/useSubscription.ts | 3 +-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.changeset/little-suits-return.md b/.changeset/little-suits-return.md index 9206b615470..6232f48b7fd 100644 --- a/.changeset/little-suits-return.md +++ b/.changeset/little-suits-return.md @@ -2,4 +2,4 @@ "@apollo/client": minor --- -Reimplement `useSubscription` +Reimplement `useSubscription` to fix rules of React violations. diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index 262a26a714d..f53db65a133 100644 --- a/src/react/components/__tests__/client/Subscription.test.tsx +++ b/src/react/components/__tests__/client/Subscription.test.tsx @@ -473,7 +473,6 @@ describe("should update", () => { snapshot, snapshot: { loading, data }, } = await ProfiledContainer.takeRender(); - console.log(snapshot); expect(loading).toBeTruthy(); expect(data).toBeUndefined(); } diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index aa389db2c8e..2da583ae68c 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -48,7 +48,6 @@ import { useSyncExternalStore } from "./useSyncExternalStore.js"; * #### Consider using `onData` instead of `useEffect` * * If you want to react to incoming data, please use the `onData` option instead of `useEffect`. State updates you make inside a `useEffect` hook might cause additional rerenders, and `useEffect` is mostly meant for side effects of rendering, not as an event handler. - * That's why we provide you with `onData` as an option to `useSubscription`. * * Consider the following component: * @@ -180,7 +179,7 @@ export function useSubscription< loading: !skip, error: void 0, data: void 0, - variables: variables, + variables, }), [skip, variables] ); From 654d94964cbdae1a758dbeaf7607c21100af8117 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 3 Jul 2024 12:56:59 +0200 Subject: [PATCH 11/13] clarify comment --- src/react/hooks/useSubscription.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 2da583ae68c..3b0d7f4303d 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -47,7 +47,9 @@ import { useSyncExternalStore } from "./useSyncExternalStore.js"; * @remarks * #### Consider using `onData` instead of `useEffect` * - * If you want to react to incoming data, please use the `onData` option instead of `useEffect`. State updates you make inside a `useEffect` hook might cause additional rerenders, and `useEffect` is mostly meant for side effects of rendering, not as an event handler. + * If you want to react to incoming data, please use the `onData` option instead of `useEffect`. + * State updates you make inside a `useEffect` hook might cause additional rerenders, and `useEffect` is mostly meant for side effects of rendering, not as an event handler. + * State updates made in an event handler like `onData` might - depending on the React version - be batched and cause only a single rerender. * * Consider the following component: * From 1af05a6c7d9ee338d2acb6b929165e62f0dc3b45 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 3 Jul 2024 13:16:29 +0200 Subject: [PATCH 12/13] update size-limits --- .size-limits.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limits.json b/.size-limits.json index 09bf55362fa..957c176449a 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39604, + "dist/apollo-client.min.cjs": 39619, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32852 } From 33829ecc5946fef5c8cff06880aa61b2221d7f57 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 3 Jul 2024 13:20:30 +0200 Subject: [PATCH 13/13] fix test --- src/react/components/__tests__/client/Subscription.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index f53db65a133..ec4deaa629c 100644 --- a/src/react/components/__tests__/client/Subscription.test.tsx +++ b/src/react/components/__tests__/client/Subscription.test.tsx @@ -470,7 +470,6 @@ describe("should update", () => { { const { - snapshot, snapshot: { loading, data }, } = await ProfiledContainer.takeRender(); expect(loading).toBeTruthy();