diff --git a/.changeset/little-suits-return.md b/.changeset/little-suits-return.md new file mode 100644 index 00000000000..6232f48b7fd --- /dev/null +++ b/.changeset/little-suits-return.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": minor +--- + +Reimplement `useSubscription` to fix rules of React violations. 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 } diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index 4584913a30d..ec4deaa629c 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,74 @@ 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).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 +521,71 @@ 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).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 +616,72 @@ 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).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/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 0ba57c64346..3b0d7f4303d 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, + 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"; + /** * > Refer to the [Subscriptions](https://www.apollographql.com/docs/react/data/subscriptions/) section for a more in-depth overview of `useSubscription`. * @@ -35,11 +45,11 @@ import { useApolloClient } from "./useApolloClient.js"; * } * ``` * @remarks - * #### Subscriptions and React 18 Automatic Batching - * - * 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. + * #### Consider using `onData` instead of `useEffect` * - * 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. + * 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: * @@ -61,10 +71,6 @@ import { useApolloClient } from "./useApolloClient.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 @@ -102,24 +108,19 @@ 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); + 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 +128,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,146 +137,178 @@ export function useSubscription< } } - const [observable, setObservable] = React.useState(() => { - if (options?.skip) { - return null; - } + const { skip, fetchPolicy, shouldResubscribe, context } = options; + const variables = useDeepMemo(() => options.variables, [options.variables]); - return client.subscribe({ - query: subscription, - variables: options?.variables, - fetchPolicy: options?.fetchPolicy, - context: options?.context, - }); - }); + let [observable, setObservable] = React.useState(() => + options.skip ? null : ( + createSubscription(client, subscription, variables, fetchPolicy, context) + ) + ); - const canResetObservableRef = React.useRef(false); - React.useEffect(() => { - return () => { - canResetObservableRef.current = true; - }; - }, []); + if (skip) { + if (observable) { + setObservable((observable = null)); + } + } else if ( + !observable || + ((client !== observable.__.client || + subscription !== observable.__.query || + fetchPolicy !== observable.__.fetchPolicy || + !equal(variables, observable.__.variables)) && + (typeof shouldResubscribe === "function" ? + !!shouldResubscribe(options!) + : shouldResubscribe) !== false) + ) { + setObservable( + (observable = createSubscription( + client, + subscription, + variables, + fetchPolicy, + context + )) + ); + } - const ref = React.useRef({ client, subscription, options }); + const optionsRef = React.useRef(options); React.useEffect(() => { - let shouldResubscribe = options?.shouldResubscribe; - if (typeof shouldResubscribe === "function") { - shouldResubscribe = !!shouldResubscribe(options!); - } + optionsRef.current = options; + }); - if (options?.skip) { - if ( - !options?.skip !== !ref.current.options?.skip || - canResetObservableRef.current - ) { - setResult({ - loading: false, - data: void 0, - error: void 0, - 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 - ) { - 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, - }) - ); - canResetObservableRef.current = false; - } + const fallbackResult = React.useMemo>( + () => ({ + loading: !skip, + error: void 0, + data: void 0, + variables, + }), + [skip, variables] + ); - Object.assign(ref.current, { client, subscription, options }); - // eslint-disable-next-line react-compiler/react-compiler - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [client, subscription, options, canResetObservableRef.current]); + return useSyncExternalStore>( + React.useCallback( + (update) => { + if (!observable) { + return () => {}; + } - React.useEffect(() => { - if (!observable) { - return; - } + let subscriptionStopped = false; + const variables = observable.__.variables; + const client = observable.__.client; + const subscription = observable.subscribe({ + next(fetchResult) { + if (subscriptionStopped) { + return; + } - let subscriptionStopped = false; - const subscription = observable.subscribe({ - next(fetchResult) { - if (subscriptionStopped) { - 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, + }; + observable.__.setResult(result); + update(); - 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); + 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.__.setResult({ + 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(); + } + } + }, + }); - 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, + 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(); - } - } + }; }, - }); + [observable] + ), + () => (observable && !skip ? observable.__.result : fallbackResult) + ); +} - 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(); - }); - }; - // eslint-disable-next-line react-compiler/react-compiler - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [observable]); +function createSubscription< + TData = any, + TVariables extends OperationVariables = OperationVariables, +>( + client: ApolloClient, + query: TypedDocumentNode, + variables?: TVariables, + 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 result; + let observable: Observable> | null = null; + return Object.assign( + 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(); + }), + { + /** + * A tracking object to store details about the observable and the latest result of the subscription. + */ + __, + } + ); } diff --git a/src/testing/react/__tests__/mockSubscriptionLink.test.tsx b/src/testing/react/__tests__/mockSubscriptionLink.test.tsx index 8b26aea0dd3..31c3abe70f7 100644 --- a/src/testing/react/__tests__/mockSubscriptionLink.test.tsx +++ b/src/testing/react/__tests__/mockSubscriptionLink.test.tsx @@ -8,9 +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"); -const IS_REACT_19 = React.version.startsWith("19"); - describe("mockSubscriptionLink", () => { it("should work with multiple subscribers to the same mock websocket", async () => { const subscription = gql` @@ -65,7 +62,7 @@ describe("mockSubscriptionLink", () => { ); - const numRenders = IS_REACT_18 || IS_REACT_19 ? 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(