From 3b90234e8862ca8b131bd02467c79980af712775 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 8 Feb 2024 15:38:49 -0700 Subject: [PATCH 01/30] Write failing test for issue 11400 --- src/react/hooks/__tests__/useQuery.test.tsx | 201 +++++++++++++++++++- 1 file changed, 200 insertions(+), 1 deletion(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 072ab6d922c..78a0ddadab5 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -26,8 +26,13 @@ import { import { QueryResult } from "../../types/types"; import { useQuery } from "../useQuery"; import { useMutation } from "../useMutation"; -import { profileHook, spyOnConsole } from "../../../testing/internal"; +import { + createProfiler, + profileHook, + spyOnConsole, +} from "../../../testing/internal"; import { useApolloClient } from "../useApolloClient"; +import { useLazyQuery } from "../useLazyQuery"; describe("useQuery Hook", () => { describe("General use", () => { @@ -4018,6 +4023,200 @@ describe("useQuery Hook", () => { }); }); + // https://github.com/apollographql/apollo-client/issues/11400 + it("does not return partial data unexpectedly when one query errors, then another succeeds with overlapping data", async () => { + interface Query1 { + person: { + __typename: "Person"; + id: number; + firstName: string; + alwaysFails: boolean; + } | null; + } + + interface Query2 { + person: { __typename: "Person"; id: number; lastName: string } | null; + } + + interface Variables { + id: number; + } + + const user = userEvent.setup(); + + const query1: TypedDocumentNode = gql` + query PersonQuery1($id: ID!) { + person(id: $id) { + id + firstName + alwaysFails + } + } + `; + + const query2: TypedDocumentNode = gql` + query PersonQuery2($id: ID!) { + person(id: $id) { + id + lastName + } + } + `; + + const Profiler = createProfiler({ + initialSnapshot: { + useQueryResult: null as QueryResult | null, + useLazyQueryResult: null as QueryResult | null, + }, + }); + + const client = new ApolloClient({ + link: new MockLink([ + { + request: { query: query1, variables: { id: 1 } }, + result: { + data: { person: null }, + errors: [new GraphQLError("Intentional error")], + }, + maxUsageCount: Number.POSITIVE_INFINITY, + }, + { + request: { query: query2, variables: { id: 1 } }, + result: { + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + }, + }, + ]), + cache: new InMemoryCache(), + }); + + function App() { + const useQueryResult = useQuery(query1, { + variables: { id: 1 }, + // This is necessary to reproduce the behavior + notifyOnNetworkStatusChange: true, + }); + + const [execute, useLazyQueryResult] = useLazyQuery(query2, { + variables: { id: 1 }, + }); + + Profiler.replaceSnapshot({ useQueryResult, useLazyQueryResult }); + + return ; + } + + render(, { + wrapper: ({ children }) => ( + + {children} + + ), + }); + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + called: true, + data: undefined, + loading: true, + networkStatus: NetworkStatus.loading, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: false, + data: undefined, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: false, + data: undefined, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + await act(() => user.click(screen.getByText("Run 2nd query"))); + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + // Since we write data to the cache that does not contain all the data to + // fulfill the first query, it will try and fetch again + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: undefined, + loading: true, + networkStatus: NetworkStatus.loading, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + await expect(Profiler).not.toRerender(); + }); + describe("Refetching", () => { it("refetching with different variables", async () => { const query = gql` From 3019c02afd9d8cfcbb2d93ce52550861277d6d51 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 8 Feb 2024 16:10:56 -0700 Subject: [PATCH 02/30] Remove properties from test that would pass with fix that arent there --- src/react/hooks/__tests__/useQuery.test.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 78a0ddadab5..1c58063e89c 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4118,7 +4118,6 @@ describe("useQuery Hook", () => { const { snapshot } = await Profiler.takeRender(); expect(snapshot.useQueryResult).toMatchObject({ - called: true, data: undefined, loading: true, networkStatus: NetworkStatus.loading, @@ -4181,7 +4180,6 @@ describe("useQuery Hook", () => { expect(snapshot.useQueryResult).toMatchObject({ data: undefined, - error: undefined, loading: true, networkStatus: NetworkStatus.loading, }); From 3432591d9bcefa859eb7a33998963a9f7dc7f0a9 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 8 Feb 2024 17:04:17 -0700 Subject: [PATCH 03/30] Set data to undefined if its partial and we don't allow partial data --- src/core/ObservableQuery.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 2b42f3b044e..5723fa0cf33 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -248,6 +248,10 @@ export class ObservableQuery< networkStatus, } as ApolloQueryResult; + if (result.partial && !this.options.returnPartialData) { + result.data = void 0 as any; + } + const { fetchPolicy = "cache-first" } = this.options; if ( // These fetch policies should never deliver data from the cache, unless From 3c3e8fc481b8df890a7b6dad787b2761ea60dfe1 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 8 Feb 2024 17:06:57 -0700 Subject: [PATCH 04/30] Add comment about partial data --- src/core/ObservableQuery.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 5723fa0cf33..fd83b22b77e 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -248,6 +248,9 @@ export class ObservableQuery< networkStatus, } as ApolloQueryResult; + // QueryInfo calls cache.watch with returnPartialData set to `true` always, + // which means that cache broadcasts for some queries will store the + // lastResult with partial data even though we don't tolerate it. if (result.partial && !this.options.returnPartialData) { result.data = void 0 as any; } From 5b1853dca83a249f1210387506511168ffb8d0c7 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 8 Feb 2024 17:21:25 -0700 Subject: [PATCH 05/30] Update test to check for react version --- src/react/hooks/__tests__/useQuery.test.tsx | 44 ++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 1c58063e89c..719c935bab9 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4153,7 +4153,49 @@ describe("useQuery Hook", () => { await act(() => user.click(screen.getByText("Run 2nd query"))); - { + if (React.version.startsWith("17")) { + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); + + // React 17 contains this additional render that we do not see on React + // 18 + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: undefined, + loading: true, + networkStatus: NetworkStatus.loading, + }); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + } else { const { snapshot } = await Profiler.takeRender(); expect(snapshot.useQueryResult).toMatchObject({ From 5d70cd866f37448f660e5fb9af213d7c621befcc Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:11:19 -0700 Subject: [PATCH 06/30] Add test to ensure getCurrentResult doesn't have data loss --- src/core/__tests__/ObservableQuery.ts | 123 ++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index d25765e9c9b..6932cf3bc57 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -24,6 +24,7 @@ import { itAsync, MockLink, mockSingleLink, + MockSubscriptionLink, subscribeAndCount, wait, } from "../../testing"; @@ -2389,6 +2390,128 @@ describe("ObservableQuery", () => { } ); + it("handles multiple calls to getCurrentResult without losing data", async () => { + const query = gql` + { + greeting { + message + ... on Greeting @defer { + recipient { + name + } + } + } + } + `; + + const link = new MockSubscriptionLink(); + + const client = new ApolloClient({ + link, + cache: new InMemoryCache(), + }); + + const obs = client.watchQuery({ query }); + const stream = new ObservableStream(obs); + + setTimeout(() => { + link.simulateResult({ + result: { + data: { + greeting: { + message: "Hello world", + __typename: "Greeting", + }, + }, + hasNext: true, + }, + }); + }); + + { + const result = await stream.takeNext(); + expect(result.data).toEqual({ + greeting: { + message: "Hello world", + __typename: "Greeting", + }, + }); + } + + expect(obs.getCurrentResult().data).toEqual({ + greeting: { + message: "Hello world", + __typename: "Greeting", + }, + }); + + expect(obs.getCurrentResult().data).toEqual({ + greeting: { + message: "Hello world", + __typename: "Greeting", + }, + }); + + setTimeout(() => { + link.simulateResult( + { + result: { + incremental: [ + { + data: { + recipient: { + name: "Alice", + __typename: "Person", + }, + __typename: "Greeting", + }, + path: ["greeting"], + }, + ], + hasNext: false, + }, + }, + true + ); + }); + + { + const result = await stream.takeNext(); + expect(result.data).toEqual({ + greeting: { + message: "Hello world", + recipient: { + name: "Alice", + __typename: "Person", + }, + __typename: "Greeting", + }, + }); + } + + expect(obs.getCurrentResult().data).toEqual({ + greeting: { + message: "Hello world", + recipient: { + name: "Alice", + __typename: "Person", + }, + __typename: "Greeting", + }, + }); + + expect(obs.getCurrentResult().data).toEqual({ + greeting: { + message: "Hello world", + recipient: { + name: "Alice", + __typename: "Person", + }, + __typename: "Greeting", + }, + }); + }); + { type Result = Partial>; From 1ca66c63ff3b175d048a0a81719ed70915fa5ee5 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:12:16 -0700 Subject: [PATCH 07/30] Remove first proposed fix from ObservableQuery --- src/core/ObservableQuery.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index fd83b22b77e..2b42f3b044e 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -248,13 +248,6 @@ export class ObservableQuery< networkStatus, } as ApolloQueryResult; - // QueryInfo calls cache.watch with returnPartialData set to `true` always, - // which means that cache broadcasts for some queries will store the - // lastResult with partial data even though we don't tolerate it. - if (result.partial && !this.options.returnPartialData) { - result.data = void 0 as any; - } - const { fetchPolicy = "cache-first" } = this.options; if ( // These fetch policies should never deliver data from the cache, unless From 1d7629dd397fb3de90f8f9f4aaeda2bf3f4d0ec6 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:40:54 -0700 Subject: [PATCH 08/30] Remove part of test that performs refetch --- src/react/hooks/__tests__/useQuery.test.tsx | 39 --------------------- 1 file changed, 39 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 719c935bab9..209ff4d91b1 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4215,45 +4215,6 @@ describe("useQuery Hook", () => { }); } - // Since we write data to the cache that does not contain all the data to - // fulfill the first query, it will try and fetch again - { - const { snapshot } = await Profiler.takeRender(); - - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - loading: true, - networkStatus: NetworkStatus.loading, - }); - - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, - loading: false, - networkStatus: NetworkStatus.ready, - }); - } - - { - const { snapshot } = await Profiler.takeRender(); - - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - loading: false, - networkStatus: NetworkStatus.error, - }); - - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, - loading: false, - networkStatus: NetworkStatus.ready, - }); - } - await expect(Profiler).not.toRerender(); }); From 42fea55d72d43dee2964763e2b4e7a9a24f3c98c Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:41:18 -0700 Subject: [PATCH 09/30] Skip reporting result when cache diff is partial and returnPartailData is false --- src/core/QueryInfo.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index c9c62973d90..109c2ee9dcf 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -211,6 +211,17 @@ export class QueryInfo { setDiff(diff: Cache.DiffResult | null) { const oldDiff = this.lastDiff && this.lastDiff.diff; this.updateLastDiff(diff); + + // If we do not tolerate partial results, skip this update to prevent it + // from being reported unnecessarily. + if ( + diff && + !diff.complete && + !this.observableQuery?.options.returnPartialData + ) { + return; + } + if (!this.dirty && !equal(oldDiff && oldDiff.result, diff && diff.result)) { this.dirty = true; if (!this.notifyTimeout) { From 22355a675b1b9b0749efb0066b58487659f8c141 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:47:18 -0700 Subject: [PATCH 10/30] Add changeset --- .changeset/neat-toes-leave.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/neat-toes-leave.md diff --git a/.changeset/neat-toes-leave.md b/.changeset/neat-toes-leave.md new file mode 100644 index 00000000000..5584adab85c --- /dev/null +++ b/.changeset/neat-toes-leave.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix issue where partial data is reported to `useQuery` when using `notifyOnNetworkStatusChange` after it errors while another overlapping query succeeds. From 7ffd17e3cf7316b0c21c019cf67bba2c93232ebe Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:51:55 -0700 Subject: [PATCH 11/30] Minor tweak to comment --- src/core/QueryInfo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 109c2ee9dcf..5056df9eb59 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -213,7 +213,7 @@ export class QueryInfo { this.updateLastDiff(diff); // If we do not tolerate partial results, skip this update to prevent it - // from being reported unnecessarily. + // from being reported. if ( diff && !diff.complete && From e7fb857633939cca9207bdc735323dd907086eca Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 14:52:32 -0700 Subject: [PATCH 12/30] Update size limits --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 46dd4f15ae3..8144477708f 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39043, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32550 + "dist/apollo-client.min.cjs": 39069, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32581 } From 364ec36292d6f91655a0cf53a7ad3d23faee76e9 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 12 Feb 2024 16:47:42 -0700 Subject: [PATCH 13/30] Move check above updateLastDiff --- src/core/QueryInfo.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 5056df9eb59..537617d10c7 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -209,9 +209,6 @@ export class QueryInfo { } setDiff(diff: Cache.DiffResult | null) { - const oldDiff = this.lastDiff && this.lastDiff.diff; - this.updateLastDiff(diff); - // If we do not tolerate partial results, skip this update to prevent it // from being reported. if ( @@ -222,6 +219,9 @@ export class QueryInfo { return; } + const oldDiff = this.lastDiff && this.lastDiff.diff; + this.updateLastDiff(diff); + if (!this.dirty && !equal(oldDiff && oldDiff.result, diff && diff.result)) { this.dirty = true; if (!this.notifyTimeout) { From 0e789652d74aa1f39a79b5d62c68f036fdb8b088 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 08:47:05 -0700 Subject: [PATCH 14/30] Schedule notification if cache eviction happened --- src/core/QueryInfo.ts | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 537617d10c7..6adbcf89977 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -209,6 +209,9 @@ export class QueryInfo { } setDiff(diff: Cache.DiffResult | null) { + const oldDiff = this.lastDiff && this.lastDiff.diff; + this.updateLastDiff(diff); + // If we do not tolerate partial results, skip this update to prevent it // from being reported. if ( @@ -216,17 +219,25 @@ export class QueryInfo { !diff.complete && !this.observableQuery?.options.returnPartialData ) { + // In the case of a cache eviction, the diff will become partial so we + // schedule a notification to send a network request (this.oqListener) to + // go and fetch the missing data. + if (!diff.complete && oldDiff?.complete) { + this.scheduleNotify(); + } + return; } - const oldDiff = this.lastDiff && this.lastDiff.diff; - this.updateLastDiff(diff); - if (!this.dirty && !equal(oldDiff && oldDiff.result, diff && diff.result)) { - this.dirty = true; - if (!this.notifyTimeout) { - this.notifyTimeout = setTimeout(() => this.notify(), 0); - } + this.scheduleNotify(); + } + } + + private scheduleNotify() { + this.dirty = true; + if (!this.notifyTimeout) { + this.notifyTimeout = setTimeout(() => this.notify(), 0); } } From 5cee1db7d71f4cc345091a92c74830cf05324d47 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 08:51:23 -0700 Subject: [PATCH 15/30] Remove redundant check --- src/core/QueryInfo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 6adbcf89977..4503880fb60 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -222,7 +222,7 @@ export class QueryInfo { // In the case of a cache eviction, the diff will become partial so we // schedule a notification to send a network request (this.oqListener) to // go and fetch the missing data. - if (!diff.complete && oldDiff?.complete) { + if (oldDiff?.complete) { this.scheduleNotify(); } From 3c24aa8a438a86835f6c32ae02ff55f02ddee2a0 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 08:54:53 -0700 Subject: [PATCH 16/30] Add additional context to comment --- src/core/QueryInfo.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 4503880fb60..19886810059 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -213,7 +213,12 @@ export class QueryInfo { this.updateLastDiff(diff); // If we do not tolerate partial results, skip this update to prevent it - // from being reported. + // from being reported. This prevents a situtuation where a query that + // errors and another succeeds with overlapping data does not report the + // partial data result to the errored query. + // + // See https://github.com/apollographql/apollo-client/issues/11400 for more + // information on this issue. if ( diff && !diff.complete && From 2a624b25cff6b14eb8e8987cf14f499aaf8a67c0 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 13:07:45 -0700 Subject: [PATCH 17/30] Simplify conditional in test for React 17 --- src/react/hooks/__tests__/useQuery.test.tsx | 52 +++++++-------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 209ff4d91b1..254afdc0136 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4175,45 +4175,25 @@ describe("useQuery Hook", () => { networkStatus: NetworkStatus.loading, }); } + } - { - const { snapshot } = await Profiler.takeRender(); - - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - loading: false, - networkStatus: NetworkStatus.error, - }); + const { snapshot } = await Profiler.takeRender(); - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, - loading: false, - networkStatus: NetworkStatus.ready, - }); - } - } else { - const { snapshot } = await Profiler.takeRender(); - - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - loading: false, - networkStatus: NetworkStatus.error, - }); + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, - loading: false, - networkStatus: NetworkStatus.ready, - }); - } + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); await expect(Profiler).not.toRerender(); }); From d78f1ff9f59e58df7b942333c7a2d293197cd0c9 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 13:12:20 -0700 Subject: [PATCH 18/30] Add check against getCurrentResult to ensure partial data is not set --- src/react/hooks/__tests__/useQuery.test.tsx | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 254afdc0136..651085bb000 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4188,6 +4188,21 @@ describe("useQuery Hook", () => { networkStatus: NetworkStatus.error, }); + // ensure we aren't setting a value on the observable query that contains + // the partial result + expect(snapshot.useQueryResult?.observable.getCurrentResult(false)).toEqual( + { + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + errors: [new GraphQLError("Intentional error")], + loading: false, + networkStatus: NetworkStatus.error, + partial: true, + } + ); + expect(snapshot.useLazyQueryResult).toMatchObject({ called: true, data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, From b497ebd6a808d5e5da4b9214681e6b6cceb09484 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 13:14:06 -0700 Subject: [PATCH 19/30] Remove unnecessary setTimeouts wrapped around simulateResult --- src/core/__tests__/ObservableQuery.ts | 54 +++++++++++++-------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 6932cf3bc57..98c1f735026 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -2414,18 +2414,16 @@ describe("ObservableQuery", () => { const obs = client.watchQuery({ query }); const stream = new ObservableStream(obs); - setTimeout(() => { - link.simulateResult({ - result: { - data: { - greeting: { - message: "Hello world", - __typename: "Greeting", - }, + link.simulateResult({ + result: { + data: { + greeting: { + message: "Hello world", + __typename: "Greeting", }, - hasNext: true, }, - }); + hasNext: true, + }, }); { @@ -2452,28 +2450,26 @@ describe("ObservableQuery", () => { }, }); - setTimeout(() => { - link.simulateResult( - { - result: { - incremental: [ - { - data: { - recipient: { - name: "Alice", - __typename: "Person", - }, - __typename: "Greeting", + link.simulateResult( + { + result: { + incremental: [ + { + data: { + recipient: { + name: "Alice", + __typename: "Person", }, - path: ["greeting"], + __typename: "Greeting", }, - ], - hasNext: false, - }, + path: ["greeting"], + }, + ], + hasNext: false, }, - true - ); - }); + }, + true + ); { const result = await stream.takeNext(); From fcf33f2d646c0d2052f941a87a330355da227d3e Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 13:17:02 -0700 Subject: [PATCH 20/30] Update size limits --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 8144477708f..b44b2372489 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39069, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32581 + "dist/apollo-client.min.cjs": 39086, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32599 } From d4f5bf82c0d46e92fe3a04402875392fe3df062d Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 13:31:53 -0700 Subject: [PATCH 21/30] Add additional checks in test to show failed situation --- src/react/hooks/__tests__/useQuery.test.tsx | 129 ++++++++++++++++---- 1 file changed, 107 insertions(+), 22 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 651085bb000..d2b32629ad4 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4103,7 +4103,21 @@ describe("useQuery Hook", () => { Profiler.replaceSnapshot({ useQueryResult, useLazyQueryResult }); - return ; + return ( + <> + + + + ); } render(, { @@ -4177,21 +4191,23 @@ describe("useQuery Hook", () => { } } - const { snapshot } = await Profiler.takeRender(); + { + const { snapshot } = await Profiler.takeRender(); - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - loading: false, - networkStatus: NetworkStatus.error, - }); + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); - // ensure we aren't setting a value on the observable query that contains - // the partial result - expect(snapshot.useQueryResult?.observable.getCurrentResult(false)).toEqual( - { + // ensure we aren't setting a value on the observable query that contains + // the partial result + expect( + snapshot.useQueryResult?.observable.getCurrentResult(false) + ).toEqual({ data: undefined, error: new ApolloError({ graphQLErrors: [new GraphQLError("Intentional error")], @@ -4200,15 +4216,84 @@ describe("useQuery Hook", () => { loading: false, networkStatus: NetworkStatus.error, partial: true, - } - ); + }); - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, - loading: false, - networkStatus: NetworkStatus.ready, - }); + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + await act(() => user.click(screen.getByText("Reload 1st query"))); + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + loading: true, + networkStatus: NetworkStatus.refetch, + }); + + // ensure we aren't setting a value on the observable query that contains + // the partial result + expect( + snapshot.useQueryResult?.observable.getCurrentResult(false) + ).toEqual({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + errors: [new GraphQLError("Intentional error")], + loading: true, + networkStatus: NetworkStatus.refetch, + partial: true, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + loading: false, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + networkStatus: NetworkStatus.error, + }); + + // ensure we aren't setting a value on the observable query that contains + // the partial result + expect( + snapshot.useQueryResult?.observable.getCurrentResult(false) + ).toEqual({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + errors: [new GraphQLError("Intentional error")], + loading: false, + networkStatus: NetworkStatus.error, + partial: true, + }); + + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } await expect(Profiler).not.toRerender(); }); From c2afd2a69fa8d2610298e36cb16e82b7631f59f1 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 14:36:38 -0700 Subject: [PATCH 22/30] Account for extra React 17 render --- src/react/hooks/__tests__/useQuery.test.tsx | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index d2b32629ad4..a0009350d61 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4228,28 +4228,13 @@ describe("useQuery Hook", () => { await act(() => user.click(screen.getByText("Reload 1st query"))); - { + if (React.version.startsWith("17")) { const { snapshot } = await Profiler.takeRender(); expect(snapshot.useQueryResult).toMatchObject({ data: undefined, loading: true, - networkStatus: NetworkStatus.refetch, - }); - - // ensure we aren't setting a value on the observable query that contains - // the partial result - expect( - snapshot.useQueryResult?.observable.getCurrentResult(false) - ).toEqual({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - errors: [new GraphQLError("Intentional error")], - loading: true, - networkStatus: NetworkStatus.refetch, - partial: true, + networkStatus: NetworkStatus.loading, }); expect(snapshot.useLazyQueryResult).toMatchObject({ From 1e8184bb8fd23326af3b7a841f370e8b5bdf950e Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 14:36:47 -0700 Subject: [PATCH 23/30] Conditionally set last diff when we notify --- src/core/QueryInfo.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 19886810059..cc0e54437ac 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -210,7 +210,6 @@ export class QueryInfo { setDiff(diff: Cache.DiffResult | null) { const oldDiff = this.lastDiff && this.lastDiff.diff; - this.updateLastDiff(diff); // If we do not tolerate partial results, skip this update to prevent it // from being reported. This prevents a situtuation where a query that @@ -228,12 +227,15 @@ export class QueryInfo { // schedule a notification to send a network request (this.oqListener) to // go and fetch the missing data. if (oldDiff?.complete) { + this.updateLastDiff(diff); this.scheduleNotify(); } return; } + this.updateLastDiff(diff); + if (!this.dirty && !equal(oldDiff && oldDiff.result, diff && diff.result)) { this.scheduleNotify(); } From 359a43f965faeab5452e3f65d08c1b4b9ba02066 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 13 Feb 2024 14:41:32 -0700 Subject: [PATCH 24/30] Update size limit --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index b44b2372489..2010be3109e 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39086, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32599 + "dist/apollo-client.min.cjs": 39090, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32603 } From c9b93176c52b68c1b9cabb7258c4c0caaa9094d5 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 08:56:04 -0700 Subject: [PATCH 25/30] Only schedule notification if not dirty --- src/core/QueryInfo.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index cc0e54437ac..86a6b890911 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -228,7 +228,10 @@ export class QueryInfo { // go and fetch the missing data. if (oldDiff?.complete) { this.updateLastDiff(diff); - this.scheduleNotify(); + + if (!this.dirty) { + this.scheduleNotify(); + } } return; From 73de9f5b3235e56405b0fbc164c36f86a67875fa Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 09:08:47 -0700 Subject: [PATCH 26/30] Simplify inner conditional to avoid duplicate logic --- src/core/QueryInfo.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 86a6b890911..b19ca164b3b 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -221,19 +221,12 @@ export class QueryInfo { if ( diff && !diff.complete && - !this.observableQuery?.options.returnPartialData - ) { + !this.observableQuery?.options.returnPartialData && // In the case of a cache eviction, the diff will become partial so we // schedule a notification to send a network request (this.oqListener) to // go and fetch the missing data. - if (oldDiff?.complete) { - this.updateLastDiff(diff); - - if (!this.dirty) { - this.scheduleNotify(); - } - } - + !(oldDiff && oldDiff.complete) + ) { return; } From e5033b533691f7b19ce1315a241a077937fa3851 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 09:14:50 -0700 Subject: [PATCH 27/30] Restore old if condition --- src/core/QueryInfo.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index b19ca164b3b..8863d7415ee 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -233,14 +233,10 @@ export class QueryInfo { this.updateLastDiff(diff); if (!this.dirty && !equal(oldDiff && oldDiff.result, diff && diff.result)) { - this.scheduleNotify(); - } - } - - private scheduleNotify() { - this.dirty = true; - if (!this.notifyTimeout) { - this.notifyTimeout = setTimeout(() => this.notify(), 0); + this.dirty = true; + if (!this.notifyTimeout) { + this.notifyTimeout = setTimeout(() => this.notify(), 0); + } } } From 449b6a24db28c6fb147087eb0990fd03e003567b Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 09:19:20 -0700 Subject: [PATCH 28/30] Add delays to remove React 17 conditional --- src/react/hooks/__tests__/useQuery.test.tsx | 40 ++++++++++----------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index a0009350d61..072c3c9e5cb 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -4079,12 +4079,14 @@ describe("useQuery Hook", () => { errors: [new GraphQLError("Intentional error")], }, maxUsageCount: Number.POSITIVE_INFINITY, + delay: 20, }, { request: { query: query2, variables: { id: 1 } }, result: { data: { person: { __typename: "Person", id: 1, lastName: "Doe" } }, }, + delay: 20, }, ]), cache: new InMemoryCache(), @@ -4167,28 +4169,24 @@ describe("useQuery Hook", () => { await act(() => user.click(screen.getByText("Run 2nd query"))); - if (React.version.startsWith("17")) { - { - const { snapshot } = await Profiler.takeRender(); + { + const { snapshot } = await Profiler.takeRender(); - expect(snapshot.useQueryResult).toMatchObject({ - data: undefined, - error: new ApolloError({ - graphQLErrors: [new GraphQLError("Intentional error")], - }), - loading: false, - networkStatus: NetworkStatus.error, - }); + expect(snapshot.useQueryResult).toMatchObject({ + data: undefined, + error: new ApolloError({ + graphQLErrors: [new GraphQLError("Intentional error")], + }), + loading: false, + networkStatus: NetworkStatus.error, + }); - // React 17 contains this additional render that we do not see on React - // 18 - expect(snapshot.useLazyQueryResult).toMatchObject({ - called: true, - data: undefined, - loading: true, - networkStatus: NetworkStatus.loading, - }); - } + expect(snapshot.useLazyQueryResult).toMatchObject({ + called: true, + data: undefined, + loading: true, + networkStatus: NetworkStatus.loading, + }); } { @@ -4228,7 +4226,7 @@ describe("useQuery Hook", () => { await act(() => user.click(screen.getByText("Reload 1st query"))); - if (React.version.startsWith("17")) { + { const { snapshot } = await Profiler.takeRender(); expect(snapshot.useQueryResult).toMatchObject({ From aa1dbd8b368fff4c937046fb7a52f91ad0bfefcd Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 10:19:37 -0700 Subject: [PATCH 29/30] Update size limits --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 2010be3109e..8d2f9f6c9d7 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39090, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32603 + "dist/apollo-client.min.cjs": 39075, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32584 } From 28960043fd31a08b692bcfeb51f0a9978fc279b1 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 14 Feb 2024 10:21:02 -0700 Subject: [PATCH 30/30] Add additional changeset --- .changeset/seven-shoes-invite.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/seven-shoes-invite.md diff --git a/.changeset/seven-shoes-invite.md b/.changeset/seven-shoes-invite.md new file mode 100644 index 00000000000..dc3363b894e --- /dev/null +++ b/.changeset/seven-shoes-invite.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where a partial cache write for an errored query would result in automatically refetching that query.