From b9d81376326dc90f1c6758b5baf7675f44e74922 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 3 Jul 2024 15:55:15 +0200 Subject: [PATCH 01/10] add `ignoreResults` option to `useSubscription` --- .../hooks/__tests__/useSubscription.test.tsx | 169 +++++++++++++++++- src/react/hooks/useSubscription.ts | 22 ++- src/react/types/types.documentation.ts | 4 + src/react/types/types.ts | 15 +- 4 files changed, 190 insertions(+), 20 deletions(-) diff --git a/src/react/hooks/__tests__/useSubscription.test.tsx b/src/react/hooks/__tests__/useSubscription.test.tsx index decdd17b973..48ef820f884 100644 --- a/src/react/hooks/__tests__/useSubscription.test.tsx +++ b/src/react/hooks/__tests__/useSubscription.test.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { renderHook, waitFor } from "@testing-library/react"; +import { render, renderHook, waitFor } from "@testing-library/react"; import gql from "graphql-tag"; import { @@ -14,7 +14,8 @@ import { InMemoryCache as Cache } from "../../../cache"; import { ApolloProvider } from "../../context"; import { MockSubscriptionLink } from "../../../testing"; import { useSubscription } from "../useSubscription"; -import { spyOnConsole } from "../../../testing/internal"; +import { profileHook, spyOnConsole } from "../../../testing/internal"; +import { SubscriptionHookOptions } from "../../types/types"; describe("useSubscription Hook", () => { it("should handle a simple subscription properly", async () => { @@ -1122,6 +1123,170 @@ followed by new in-flight setup", async () => { }); }); +describe("ignoreResults", () => { + it("should not rerender when ignoreResults is true, but will call `onData` and `onComplete`", async () => { + const subscription = gql` + subscription { + car { + make + } + } + `; + + const results = ["Audi", "BMW"].map((make) => ({ + result: { data: { car: { make } } }, + })); + + const link = new MockSubscriptionLink(); + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); + + const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); + const onError = jest.fn((() => {}) as SubscriptionHookOptions["onError"]); + const onComplete = jest.fn( + (() => {}) as SubscriptionHookOptions["onComplete"] + ); + const ProfiledHook = profileHook(() => + useSubscription(subscription, { + ignoreResults: true, + onData, + onError, + onComplete, + }) + ); + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + data: undefined, + variables: undefined, + }); + link.simulateResult(results[0]); + + await waitFor(() => { + expect(onData).toHaveBeenCalledTimes(1); + expect(onData.mock.lastCall?.[0].data).toStrictEqual({ + data: results[0].result.data, + error: undefined, + loading: false, + variables: undefined, + }); + expect(onError).toHaveBeenCalledTimes(0); + expect(onComplete).toHaveBeenCalledTimes(0); + }); + + link.simulateResult(results[1], true); + await waitFor(() => { + expect(onData).toHaveBeenCalledTimes(2); + expect(onData.mock.lastCall?.[0].data).toStrictEqual({ + data: results[1].result.data, + error: undefined, + loading: false, + variables: undefined, + }); + expect(onError).toHaveBeenCalledTimes(0); + expect(onComplete).toHaveBeenCalledTimes(1); + }); + + // const error = new Error("test"); + // link.simulateResult({ error }); + // await waitFor(() => { + // expect(onData).toHaveBeenCalledTimes(2); + // expect(onError).toHaveBeenCalledTimes(1); + // expect(onError).toHaveBeenLastCalledWith(error); + // expect(onComplete).toHaveBeenCalledTimes(0); + // }); + + await expect(ProfiledHook).not.toRerender(); + }); + + it("should not rerender when ignoreResults is true and an error occurs", async () => { + const subscription = gql` + subscription { + car { + make + } + } + `; + + const results = ["Audi", "BMW"].map((make) => ({ + result: { data: { car: { make } } }, + })); + + const link = new MockSubscriptionLink(); + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); + + const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); + const onError = jest.fn((() => {}) as SubscriptionHookOptions["onError"]); + const onComplete = jest.fn( + (() => {}) as SubscriptionHookOptions["onComplete"] + ); + const ProfiledHook = profileHook(() => + useSubscription(subscription, { + ignoreResults: true, + onData, + onError, + onComplete, + }) + ); + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + data: undefined, + variables: undefined, + }); + link.simulateResult(results[0]); + + await waitFor(() => { + expect(onData).toHaveBeenCalledTimes(1); + expect(onData.mock.lastCall?.[0].data).toStrictEqual({ + data: results[0].result.data, + error: undefined, + loading: false, + variables: undefined, + }); + expect(onError).toHaveBeenCalledTimes(0); + expect(onComplete).toHaveBeenCalledTimes(0); + }); + + const error = new Error("test"); + link.simulateResult({ error }); + await waitFor(() => { + expect(onData).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenLastCalledWith(error); + expect(onComplete).toHaveBeenCalledTimes(0); + }); + + await expect(ProfiledHook).not.toRerender(); + }); + + it.todo( + "can switch from `ignoreResults: true` to `ignoreResults: false` and will start rerendering, without creating a new subscription" + ); + it.todo( + "can switch from `ignoreResults: false` to `ignoreResults: true` and will stop rerendering, without creating a new subscription" + ); +}); + describe.skip("Type Tests", () => { test("NoInfer prevents adding arbitrary additional variables", () => { const typedNode = {} as TypedDocumentNode<{ foo: string }, { bar: number }>; diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 3b0d7f4303d..851e7b206c2 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -137,7 +137,8 @@ export function useSubscription< } } - const { skip, fetchPolicy, shouldResubscribe, context } = options; + const { skip, fetchPolicy, shouldResubscribe, context, ignoreResults } = + options; const variables = useDeepMemo(() => options.variables, [options.variables]); let [observable, setObservable] = React.useState(() => @@ -176,14 +177,15 @@ export function useSubscription< optionsRef.current = options; }); + const fallbackLoading = !skip && !ignoreResults; const fallbackResult = React.useMemo>( () => ({ - loading: !skip, + loading: fallbackLoading, error: void 0, data: void 0, variables, }), - [skip, variables] + [fallbackLoading, variables] ); return useSyncExternalStore>( @@ -211,7 +213,7 @@ export function useSubscription< variables, }; observable.__.setResult(result); - update(); + if (!ignoreResults) update(); if (optionsRef.current.onData) { optionsRef.current.onData({ @@ -233,7 +235,7 @@ export function useSubscription< error, variables, }); - update(); + if (!ignoreResults) update(); optionsRef.current.onError?.(error); } }, @@ -250,7 +252,8 @@ export function useSubscription< return () => { // immediately stop receiving subscription values, but do not unsubscribe - // until after a short delay in case another useSubscription hook is + // until after a short delay in case another useSubscription hook + // (or this hook, after flipping `ignoreResults`) is // reusing the same underlying observable and is about to subscribe subscriptionStopped = true; setTimeout(() => { @@ -258,9 +261,12 @@ export function useSubscription< }); }; }, - [observable] + [ignoreResults, observable] ), - () => (observable && !skip ? observable.__.result : fallbackResult) + () => + observable && !skip && !ignoreResults ? + observable.__.result + : fallbackResult ); } diff --git a/src/react/types/types.documentation.ts b/src/react/types/types.documentation.ts index 186d651dfd8..9c0236cdaab 100644 --- a/src/react/types/types.documentation.ts +++ b/src/react/types/types.documentation.ts @@ -531,6 +531,10 @@ export interface SubscriptionOptionsDocumentation { */ shouldResubscribe: unknown; + /** + * If `true`, the hook will not cause a component to rerender - this is useful when you want to control the rendering of your component yourself with logic in the `onData` and `onError` callbacks. + */ + ignoreResults: unknown; /** * An `ApolloClient` instance. By default `useSubscription` / `Subscription` uses the client passed down via context, but a different client can be passed in. */ diff --git a/src/react/types/types.ts b/src/react/types/types.ts index 41cff9e8835..25cd1ff66d6 100644 --- a/src/react/types/types.ts +++ b/src/react/types/types.ts @@ -457,6 +457,11 @@ export interface BaseSubscriptionOptions< onError?: (error: ApolloError) => void; /** {@inheritDoc @apollo/client!SubscriptionOptionsDocumentation#onSubscriptionComplete:member} */ onSubscriptionComplete?: () => void; + /** + * {@inheritDoc @apollo/client!SubscriptionOptionsDocumentation#ignoreResults:member} + * @default false + */ + ignoreResults?: boolean; } export interface SubscriptionResult { @@ -479,16 +484,6 @@ export interface SubscriptionHookOptions< TVariables extends OperationVariables = OperationVariables, > extends BaseSubscriptionOptions {} -export interface SubscriptionDataOptions< - TData = any, - TVariables extends OperationVariables = OperationVariables, -> extends BaseSubscriptionOptions { - subscription: DocumentNode | TypedDocumentNode; - children?: - | null - | ((result: SubscriptionResult) => ReactTypes.ReactNode); -} - export interface SubscriptionCurrentObservable { query?: Observable; subscription?: ObservableSubscription; From 8d07bbca8e2fb29ab9a4b28a6ca91c534d480fad Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Jul 2024 11:59:28 +0200 Subject: [PATCH 02/10] more tests --- .../hooks/__tests__/useSubscription.test.tsx | 181 ++++++++++++++---- src/react/hooks/useSubscription.ts | 20 +- 2 files changed, 160 insertions(+), 41 deletions(-) diff --git a/src/react/hooks/__tests__/useSubscription.test.tsx b/src/react/hooks/__tests__/useSubscription.test.tsx index 48ef820f884..42d02a52f90 100644 --- a/src/react/hooks/__tests__/useSubscription.test.tsx +++ b/src/react/hooks/__tests__/useSubscription.test.tsx @@ -16,7 +16,6 @@ import { MockSubscriptionLink } from "../../../testing"; import { useSubscription } from "../useSubscription"; import { profileHook, spyOnConsole } from "../../../testing/internal"; import { SubscriptionHookOptions } from "../../types/types"; - describe("useSubscription Hook", () => { it("should handle a simple subscription properly", async () => { const subscription = gql` @@ -1124,19 +1123,19 @@ followed by new in-flight setup", async () => { }); describe("ignoreResults", () => { - it("should not rerender when ignoreResults is true, but will call `onData` and `onComplete`", async () => { - const subscription = gql` - subscription { - car { - make - } + const subscription = gql` + subscription { + car { + make } - `; + } + `; - const results = ["Audi", "BMW"].map((make) => ({ - result: { data: { car: { make } } }, - })); + const results = ["Audi", "BMW"].map((make) => ({ + result: { data: { car: { make } } }, + })); + it("should not rerender when ignoreResults is true, but will call `onData` and `onComplete`", async () => { const link = new MockSubscriptionLink(); const client = new ApolloClient({ link, @@ -1196,31 +1195,10 @@ describe("ignoreResults", () => { expect(onComplete).toHaveBeenCalledTimes(1); }); - // const error = new Error("test"); - // link.simulateResult({ error }); - // await waitFor(() => { - // expect(onData).toHaveBeenCalledTimes(2); - // expect(onError).toHaveBeenCalledTimes(1); - // expect(onError).toHaveBeenLastCalledWith(error); - // expect(onComplete).toHaveBeenCalledTimes(0); - // }); - await expect(ProfiledHook).not.toRerender(); }); it("should not rerender when ignoreResults is true and an error occurs", async () => { - const subscription = gql` - subscription { - car { - make - } - } - `; - - const results = ["Audi", "BMW"].map((make) => ({ - result: { data: { car: { make } } }, - })); - const link = new MockSubscriptionLink(); const client = new ApolloClient({ link, @@ -1279,12 +1257,139 @@ describe("ignoreResults", () => { await expect(ProfiledHook).not.toRerender(); }); - it.todo( - "can switch from `ignoreResults: true` to `ignoreResults: false` and will start rerendering, without creating a new subscription" - ); - it.todo( - "can switch from `ignoreResults: false` to `ignoreResults: true` and will stop rerendering, without creating a new subscription" - ); + it("can switch from `ignoreResults: true` to `ignoreResults: false` and will start rerendering, without creating a new subscription", async () => { + const subscriptionCreated = jest.fn(); + const link = new MockSubscriptionLink(); + link.onSetup(subscriptionCreated); + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); + + const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); + const ProfiledHook = profileHook( + ({ ignoreResults }: { ignoreResults: boolean }) => + useSubscription(subscription, { + ignoreResults, + onData, + }) + ); + const { rerender } = render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + expect(subscriptionCreated).toHaveBeenCalledTimes(1); + + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + data: undefined, + variables: undefined, + }); + expect(onData).toHaveBeenCalledTimes(0); + } + link.simulateResult(results[0]); + await expect(ProfiledHook).not.toRerender({ timeout: 20 }); + + rerender(); + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + // `data` appears immediately after changing to `ignoreResults: false` + data: results[0].result.data, + variables: undefined, + }); + // `onData` should not be called again for the same result + expect(onData).toHaveBeenCalledTimes(1); + } + + link.simulateResult(results[1]); + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + data: results[1].result.data, + variables: undefined, + }); + expect(onData).toHaveBeenCalledTimes(2); + } + // a second subscription should not have been started + expect(subscriptionCreated).toHaveBeenCalledTimes(1); + }); + it("can switch from `ignoreResults: false` to `ignoreResults: true` and will stop rerendering, without creating a new subscription", async () => { + const subscriptionCreated = jest.fn(); + const link = new MockSubscriptionLink(); + link.onSetup(subscriptionCreated); + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); + + const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); + const ProfiledHook = profileHook( + ({ ignoreResults }: { ignoreResults: boolean }) => + useSubscription(subscription, { + ignoreResults, + onData, + }) + ); + const { rerender } = render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + expect(subscriptionCreated).toHaveBeenCalledTimes(1); + + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: true, + error: undefined, + data: undefined, + variables: undefined, + }); + expect(onData).toHaveBeenCalledTimes(0); + } + link.simulateResult(results[0]); + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + data: results[0].result.data, + variables: undefined, + }); + expect(onData).toHaveBeenCalledTimes(1); + } + await expect(ProfiledHook).not.toRerender({ timeout: 20 }); + + rerender(); + { + const snapshot = await ProfiledHook.takeSnapshot(); + expect(snapshot).toStrictEqual({ + loading: false, + error: undefined, + // switching back to the default `ignoreResults: true` return value + data: undefined, + variables: undefined, + }); + // `onData` should not be called again + expect(onData).toHaveBeenCalledTimes(1); + } + + link.simulateResult(results[1]); + await expect(ProfiledHook).not.toRerender({ timeout: 20 }); + expect(onData).toHaveBeenCalledTimes(2); + + // a second subscription should not have been started + expect(subscriptionCreated).toHaveBeenCalledTimes(1); + }); }); describe.skip("Type Tests", () => { diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 851e7b206c2..21dbeb94a14 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -21,6 +21,7 @@ import { Observable } from "../../core/index.js"; import { useApolloClient } from "./useApolloClient.js"; import { useDeepMemo } from "./internal/useDeepMemo.js"; import { useSyncExternalStore } from "./useSyncExternalStore.js"; +import { useIsomorphicLayoutEffect } from "./internal/useIsomorphicLayoutEffect.js"; /** * > Refer to the [Subscriptions](https://www.apollographql.com/docs/react/data/subscriptions/) section for a more in-depth overview of `useSubscription`. @@ -188,6 +189,19 @@ export function useSubscription< [fallbackLoading, variables] ); + const ignoreResultsRef = React.useRef(ignoreResults); + useIsomorphicLayoutEffect(() => { + // We cannot directly reference `ignoreResults` directly in the effect below + // it would add a dependency to the `useEffect` deps array, which means the + // subscription would be recreated if `ignoreResults` changes + // As a result, on resubscription, the last result would be re-delivered, + // rendering the component one additional time, and re-triggering `onData`. + // The same applies to `fetchPolicy`, which results in a new `observable` + // being created. We cannot really avoid it in that case, but we can at least + // avoid it for `ignoreResults`. + ignoreResultsRef.current = ignoreResults; + }); + return useSyncExternalStore>( React.useCallback( (update) => { @@ -213,7 +227,7 @@ export function useSubscription< variables, }; observable.__.setResult(result); - if (!ignoreResults) update(); + if (!ignoreResultsRef.current) update(); if (optionsRef.current.onData) { optionsRef.current.onData({ @@ -235,7 +249,7 @@ export function useSubscription< error, variables, }); - if (!ignoreResults) update(); + if (!ignoreResultsRef.current) update(); optionsRef.current.onError?.(error); } }, @@ -261,7 +275,7 @@ export function useSubscription< }); }; }, - [ignoreResults, observable] + [observable] ), () => observable && !skip && !ignoreResults ? From 513c1e7ad8d10e30e3b2886f506a2781b9bb42ba Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Jul 2024 12:00:11 +0200 Subject: [PATCH 03/10] changeset --- .changeset/unlucky-birds-press.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/unlucky-birds-press.md diff --git a/.changeset/unlucky-birds-press.md b/.changeset/unlucky-birds-press.md new file mode 100644 index 00000000000..5696787576d --- /dev/null +++ b/.changeset/unlucky-birds-press.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +add `ignoreResults` option to `useSubscription` From 45483fc5ecf19ce0cef9b58d1ce1711f850a1797 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Jul 2024 12:16:47 +0200 Subject: [PATCH 04/10] restore type, add deprecation, tweak tag --- .api-reports/api-report-react.api.md | 3 ++- .api-reports/api-report-react_components.api.md | 1 + .api-reports/api-report-react_hooks.api.md | 1 + .api-reports/api-report.api.md | 3 ++- .size-limits.json | 2 +- src/react/types/types.ts | 15 ++++++++++++++- 6 files changed, 21 insertions(+), 4 deletions(-) diff --git a/.api-reports/api-report-react.api.md b/.api-reports/api-report-react.api.md index 74721b26f76..82dc5dfdff8 100644 --- a/.api-reports/api-report-react.api.md +++ b/.api-reports/api-report-react.api.md @@ -384,6 +384,7 @@ export interface BaseSubscriptionOptions void; onData?: (options: OnDataOptions) => any; onError?: (error: ApolloError) => void; @@ -1915,7 +1916,7 @@ export interface SubscriptionCurrentObservable { subscription?: Subscription; } -// @public (undocumented) +// @public @deprecated (undocumented) export interface SubscriptionDataOptions extends BaseSubscriptionOptions { // (undocumented) children?: null | ((result: SubscriptionResult) => ReactTypes.ReactNode); diff --git a/.api-reports/api-report-react_components.api.md b/.api-reports/api-report-react_components.api.md index b9bd5eda67e..7cac2e1da09 100644 --- a/.api-reports/api-report-react_components.api.md +++ b/.api-reports/api-report-react_components.api.md @@ -332,6 +332,7 @@ interface BaseSubscriptionOptions void; // Warning: (ae-forgotten-export) The symbol "OnDataOptions" needs to be exported by the entry point index.d.ts onData?: (options: OnDataOptions) => any; diff --git a/.api-reports/api-report-react_hooks.api.md b/.api-reports/api-report-react_hooks.api.md index 9a8d8096055..6d60938847f 100644 --- a/.api-reports/api-report-react_hooks.api.md +++ b/.api-reports/api-report-react_hooks.api.md @@ -355,6 +355,7 @@ interface BaseSubscriptionOptions void; // Warning: (ae-forgotten-export) The symbol "OnDataOptions" needs to be exported by the entry point index.d.ts onData?: (options: OnDataOptions) => any; diff --git a/.api-reports/api-report.api.md b/.api-reports/api-report.api.md index 95cc0a4694b..4dc88e63362 100644 --- a/.api-reports/api-report.api.md +++ b/.api-reports/api-report.api.md @@ -355,6 +355,7 @@ export interface BaseSubscriptionOptions; context?: DefaultContext; fetchPolicy?: FetchPolicy; + ignoreResults?: boolean; onComplete?: () => void; onData?: (options: OnDataOptions) => any; onError?: (error: ApolloError) => void; @@ -2547,7 +2548,7 @@ export interface SubscriptionCurrentObservable { subscription?: ObservableSubscription; } -// @public (undocumented) +// @public @deprecated (undocumented) export interface SubscriptionDataOptions extends BaseSubscriptionOptions { // (undocumented) children?: null | ((result: SubscriptionResult) => ReactTypes.ReactNode); diff --git a/.size-limits.json b/.size-limits.json index 957c176449a..90a9c60ba42 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39619, + "dist/apollo-client.min.cjs": 39641, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32852 } diff --git a/src/react/types/types.ts b/src/react/types/types.ts index 25cd1ff66d6..be799bf52dd 100644 --- a/src/react/types/types.ts +++ b/src/react/types/types.ts @@ -459,7 +459,7 @@ export interface BaseSubscriptionOptions< onSubscriptionComplete?: () => void; /** * {@inheritDoc @apollo/client!SubscriptionOptionsDocumentation#ignoreResults:member} - * @default false + * @defaultValue `false` */ ignoreResults?: boolean; } @@ -484,6 +484,19 @@ export interface SubscriptionHookOptions< TVariables extends OperationVariables = OperationVariables, > extends BaseSubscriptionOptions {} +/** + * @deprecated This type is not used anymore. It will be removed in the next major version of Apollo Client + */ +export interface SubscriptionDataOptions< + TData = any, + TVariables extends OperationVariables = OperationVariables, +> extends BaseSubscriptionOptions { + subscription: DocumentNode | TypedDocumentNode; + children?: + | null + | ((result: SubscriptionResult) => ReactTypes.ReactNode); +} + export interface SubscriptionCurrentObservable { query?: Observable; subscription?: ObservableSubscription; From 482117a2ce002f7b96f5dd346f400254976cf905 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Jul 2024 12:24:21 +0200 Subject: [PATCH 05/10] Update src/react/hooks/useSubscription.ts --- src/react/hooks/useSubscription.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 21dbeb94a14..1b741f98dec 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -191,7 +191,7 @@ export function useSubscription< const ignoreResultsRef = React.useRef(ignoreResults); useIsomorphicLayoutEffect(() => { - // We cannot directly reference `ignoreResults` directly in the effect below + // We cannot reference `ignoreResults` directly in the effect below // it would add a dependency to the `useEffect` deps array, which means the // subscription would be recreated if `ignoreResults` changes // As a result, on resubscription, the last result would be re-delivered, From d29b33ce4570b74655e7324d4849811419c45c50 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Jul 2024 12:25:49 +0200 Subject: [PATCH 06/10] reflect code change in comment --- src/react/hooks/useSubscription.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts index 1b741f98dec..76892692233 100644 --- a/src/react/hooks/useSubscription.ts +++ b/src/react/hooks/useSubscription.ts @@ -266,8 +266,7 @@ export function useSubscription< return () => { // immediately stop receiving subscription values, but do not unsubscribe - // until after a short delay in case another useSubscription hook - // (or this hook, after flipping `ignoreResults`) is + // until after a short delay in case another useSubscription hook is // reusing the same underlying observable and is about to subscribe subscriptionStopped = true; setTimeout(() => { From 2a7f228da3fc5c4c8f0f80f75e91dae021c57647 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 8 Jul 2024 12:14:25 +0200 Subject: [PATCH 07/10] review feedback --- .../hooks/__tests__/useSubscription.test.tsx | 49 ++++++++++++------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/src/react/hooks/__tests__/useSubscription.test.tsx b/src/react/hooks/__tests__/useSubscription.test.tsx index 42d02a52f90..6081cf87ac4 100644 --- a/src/react/hooks/__tests__/useSubscription.test.tsx +++ b/src/react/hooks/__tests__/useSubscription.test.tsx @@ -1172,12 +1172,16 @@ describe("ignoreResults", () => { await waitFor(() => { expect(onData).toHaveBeenCalledTimes(1); - expect(onData.mock.lastCall?.[0].data).toStrictEqual({ - data: results[0].result.data, - error: undefined, - loading: false, - variables: undefined, - }); + expect(onData).toHaveBeenLastCalledWith( + expect.objectContaining({ + data: { + data: results[0].result.data, + error: undefined, + loading: false, + variables: undefined, + }, + }) + ); expect(onError).toHaveBeenCalledTimes(0); expect(onComplete).toHaveBeenCalledTimes(0); }); @@ -1185,12 +1189,16 @@ describe("ignoreResults", () => { link.simulateResult(results[1], true); await waitFor(() => { expect(onData).toHaveBeenCalledTimes(2); - expect(onData.mock.lastCall?.[0].data).toStrictEqual({ - data: results[1].result.data, - error: undefined, - loading: false, - variables: undefined, - }); + expect(onData).toHaveBeenLastCalledWith( + expect.objectContaining({ + data: { + data: results[1].result.data, + error: undefined, + loading: false, + variables: undefined, + }, + }) + ); expect(onError).toHaveBeenCalledTimes(0); expect(onComplete).toHaveBeenCalledTimes(1); }); @@ -1235,12 +1243,16 @@ describe("ignoreResults", () => { await waitFor(() => { expect(onData).toHaveBeenCalledTimes(1); - expect(onData.mock.lastCall?.[0].data).toStrictEqual({ - data: results[0].result.data, - error: undefined, - loading: false, - variables: undefined, - }); + expect(onData).toHaveBeenLastCalledWith( + expect.objectContaining({ + data: { + data: results[0].result.data, + error: undefined, + loading: false, + variables: undefined, + }, + }) + ); expect(onError).toHaveBeenCalledTimes(0); expect(onComplete).toHaveBeenCalledTimes(0); }); @@ -1293,6 +1305,7 @@ describe("ignoreResults", () => { } link.simulateResult(results[0]); await expect(ProfiledHook).not.toRerender({ timeout: 20 }); + expect(onData).toHaveBeenCalledTimes(1); rerender(); { From d7afc64a8910b341531ad37e5cff7d0092553335 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 8 Jul 2024 12:16:57 +0200 Subject: [PATCH 08/10] Update src/react/types/types.documentation.ts Co-authored-by: Jerel Miller --- src/react/types/types.documentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/react/types/types.documentation.ts b/src/react/types/types.documentation.ts index 9c0236cdaab..e4aa37565d2 100644 --- a/src/react/types/types.documentation.ts +++ b/src/react/types/types.documentation.ts @@ -532,7 +532,7 @@ export interface SubscriptionOptionsDocumentation { shouldResubscribe: unknown; /** - * If `true`, the hook will not cause a component to rerender - this is useful when you want to control the rendering of your component yourself with logic in the `onData` and `onError` callbacks. + * If `true`, the hook will not cause the component to rerender. This is useful when you want to control the rendering of your component yourself with logic in the `onData` and `onError` callbacks. */ ignoreResults: unknown; /** From cc41b89e78637ea2508f82faf791710494cb8f05 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 8 Jul 2024 12:31:52 +0200 Subject: [PATCH 09/10] add clarification about resetting the return value when switching on `ignoreResults` later --- src/react/types/types.documentation.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/react/types/types.documentation.ts b/src/react/types/types.documentation.ts index e4aa37565d2..c5f232c1b18 100644 --- a/src/react/types/types.documentation.ts +++ b/src/react/types/types.documentation.ts @@ -533,6 +533,8 @@ export interface SubscriptionOptionsDocumentation { /** * If `true`, the hook will not cause the component to rerender. This is useful when you want to control the rendering of your component yourself with logic in the `onData` and `onError` callbacks. + * + * Changing this to `true` when the hook already has `data` will reset the `data` to `undefined`. */ ignoreResults: unknown; /** From 03c973969ca2ce437393b13ac33a50bead458a16 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 8 Jul 2024 12:49:45 +0200 Subject: [PATCH 10/10] test fixup --- src/react/hooks/__tests__/useSubscription.test.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/react/hooks/__tests__/useSubscription.test.tsx b/src/react/hooks/__tests__/useSubscription.test.tsx index e3d121d228c..eb02e41c9aa 100644 --- a/src/react/hooks/__tests__/useSubscription.test.tsx +++ b/src/react/hooks/__tests__/useSubscription.test.tsx @@ -1500,6 +1500,7 @@ describe("ignoreResults", () => { error: undefined, data: undefined, variables: undefined, + restart: expect.any(Function), }); link.simulateResult(results[0]); @@ -1571,6 +1572,7 @@ describe("ignoreResults", () => { error: undefined, data: undefined, variables: undefined, + restart: expect.any(Function), }); link.simulateResult(results[0]); @@ -1633,6 +1635,7 @@ describe("ignoreResults", () => { error: undefined, data: undefined, variables: undefined, + restart: expect.any(Function), }); expect(onData).toHaveBeenCalledTimes(0); } @@ -1649,6 +1652,7 @@ describe("ignoreResults", () => { // `data` appears immediately after changing to `ignoreResults: false` data: results[0].result.data, variables: undefined, + restart: expect.any(Function), }); // `onData` should not be called again for the same result expect(onData).toHaveBeenCalledTimes(1); @@ -1662,6 +1666,7 @@ describe("ignoreResults", () => { error: undefined, data: results[1].result.data, variables: undefined, + restart: expect.any(Function), }); expect(onData).toHaveBeenCalledTimes(2); } @@ -1699,6 +1704,7 @@ describe("ignoreResults", () => { error: undefined, data: undefined, variables: undefined, + restart: expect.any(Function), }); expect(onData).toHaveBeenCalledTimes(0); } @@ -1710,6 +1716,7 @@ describe("ignoreResults", () => { error: undefined, data: results[0].result.data, variables: undefined, + restart: expect.any(Function), }); expect(onData).toHaveBeenCalledTimes(1); } @@ -1724,6 +1731,7 @@ describe("ignoreResults", () => { // switching back to the default `ignoreResults: true` return value data: undefined, variables: undefined, + restart: expect.any(Function), }); // `onData` should not be called again expect(onData).toHaveBeenCalledTimes(1);