From 7a9c93a4ac5c89a3d5e987ece9fd0fa6cfb30e39 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Tue, 6 Feb 2024 14:56:03 -0500 Subject: [PATCH 1/4] Revert "Merge pull request #11202 from apollographql/issue-9293-fix-QueryInfo-markResult-result.data-mutation" This reverts commit 8b6e10bf77aa98e65d0035e243ee41238387f7db, reversing changes made to 7c90f9a46c1af0860c4fbc3faec2d89ab7818628. --- src/__tests__/client.ts | 5 +- src/cache/inmemory/__tests__/client.ts | 169 -------------------- src/core/QueryInfo.ts | 23 ++- src/core/QueryManager.ts | 2 +- src/core/__tests__/QueryManager/index.ts | 113 ++++++++++++- src/react/hooks/__tests__/useQuery.test.tsx | 5 +- 6 files changed, 125 insertions(+), 192 deletions(-) delete mode 100644 src/cache/inmemory/__tests__/client.ts diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index 2b0e3fe74cf..cf8df19c268 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -2949,10 +2949,7 @@ describe("client", () => { return client .query({ query }) .then(({ data }) => { - const { price, ...todoWithoutPrice } = data.todos[0]; - expect(data).toEqual({ - todos: [todoWithoutPrice], - }); + expect(data).toEqual(result.data); }) .then(resolve, reject); }); diff --git a/src/cache/inmemory/__tests__/client.ts b/src/cache/inmemory/__tests__/client.ts deleted file mode 100644 index 23fd87f6f73..00000000000 --- a/src/cache/inmemory/__tests__/client.ts +++ /dev/null @@ -1,169 +0,0 @@ -// This file contains InMemoryCache-specific tests that exercise the -// ApolloClient class. Other test modules in this directory only test -// InMemoryCache and related utilities, without involving ApolloClient. - -import { ApolloClient, WatchQueryFetchPolicy, gql } from "../../../core"; -import { ApolloLink } from "../../../link/core"; -import { Observable } from "../../../utilities"; -import { InMemoryCache } from "../.."; -import { subscribeAndCount } from "../../../testing"; - -describe("InMemoryCache tests exercising ApolloClient", () => { - it.each([ - "cache-first", - "network-only", - "cache-and-network", - "cache-only", - "no-cache", - ])( - "results should be read from cache even when incomplete (fetchPolicy %s)", - (fetchPolicy) => { - const dateFromCache = "2023-09-14T13:03:22.616Z"; - const dateFromNetwork = "2023-09-15T13:03:22.616Z"; - - const cache = new InMemoryCache({ - typePolicies: { - Query: { - fields: { - date: { - read(existing) { - return new Date(existing || dateFromCache); - }, - }, - }, - }, - }, - }); - - const client = new ApolloClient({ - link: new ApolloLink( - (operation) => - new Observable((observer) => { - observer.next({ - data: { - // This raw string should be converted to a Date by the Query.date - // read function passed to InMemoryCache below. - date: dateFromNetwork, - // Make sure we don't accidentally return fields not mentioned in - // the query just because the result is incomplete. - ignored: "irrelevant to the subscribed query", - // Note the Query.missing field is, well, missing. - }, - }); - setTimeout(() => { - observer.complete(); - }, 10); - }) - ), - cache, - }); - - const query = gql` - query { - date - missing - } - `; - - const observable = client.watchQuery({ - query, - fetchPolicy, // Varies with each test iteration - returnPartialData: true, - }); - - return new Promise((resolve, reject) => { - subscribeAndCount(reject, observable, (handleCount, result) => { - let adjustedCount = handleCount; - if ( - fetchPolicy === "network-only" || - fetchPolicy === "no-cache" || - fetchPolicy === "cache-only" - ) { - // The network-only, no-cache, and cache-only fetch policies do not - // deliver a loading:true result initially, so we adjust the - // handleCount to skip that case. - ++adjustedCount; - } - - // The only fetch policy that does not re-read results from the cache is - // the "no-cache" policy. In this test, that means the Query.date field - // will remain as a raw string rather than being converted to a Date by - // the read function. - const expectedDateAfterResult = - fetchPolicy === "cache-only" ? new Date(dateFromCache) - : fetchPolicy === "no-cache" ? dateFromNetwork - : new Date(dateFromNetwork); - - if (adjustedCount === 1) { - expect(result.loading).toBe(true); - expect(result.data).toEqual({ - date: new Date(dateFromCache), - }); - } else if (adjustedCount === 2) { - expect(result.loading).toBe(false); - expect(result.data).toEqual({ - date: expectedDateAfterResult, - // The no-cache fetch policy does return extraneous fields from the - // raw network result that were not requested in the query, since - // the cache is not consulted. - ...(fetchPolicy === "no-cache" ? - { - ignored: "irrelevant to the subscribed query", - } - : null), - }); - - if (fetchPolicy === "no-cache") { - // The "no-cache" fetch policy does not receive updates from the - // cache, so we finish the test early (passing). - setTimeout(() => resolve(), 20); - } else { - cache.writeQuery({ - query: gql` - query { - missing - } - `, - data: { - missing: "not missing anymore", - }, - }); - } - } else if (adjustedCount === 3) { - expect(result.loading).toBe(false); - expect(result.data).toEqual({ - date: expectedDateAfterResult, - missing: "not missing anymore", - }); - - expect(cache.extract()).toEqual({ - ROOT_QUERY: { - __typename: "Query", - // The cache-only fetch policy does not receive updates from the - // network, so it never ends up writing the date field into the - // cache explicitly, though Query.date can still be synthesized by - // the read function. - ...(fetchPolicy === "cache-only" ? null : ( - { - // Make sure this field is stored internally as a raw string. - date: dateFromNetwork, - } - )), - // Written explicitly with cache.writeQuery above. - missing: "not missing anymore", - // The ignored field is never written to the cache, because it is - // not included in the query. - }, - }); - - // Wait 20ms to give the test a chance to fail if there are unexpected - // additional results. - setTimeout(() => resolve(), 20); - } else { - reject(new Error(`Unexpected count ${adjustedCount}`)); - } - }); - }); - } - ); -}); diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index f2aa2afa518..c9c62973d90 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -364,8 +364,7 @@ export class QueryInfo { "variables" | "fetchPolicy" | "errorPolicy" >, cacheWriteBehavior: CacheWriteBehavior - ): typeof result { - result = { ...result }; + ) { const merger = new DeepMerger(); const graphQLErrors = isNonEmptyArray(result.errors) ? result.errors.slice(0) : []; @@ -411,10 +410,7 @@ export class QueryInfo { }); this.lastWrite = { - // Make a shallow defensive copy of the result object, in case we - // later later modify result.data in place, since we don't want - // that mutation affecting the saved lastWrite.result.data. - result: { ...result }, + result, variables: options.variables, dmCount: destructiveMethodCounts.get(this.cache), }; @@ -476,19 +472,20 @@ export class QueryInfo { this.updateWatch(options.variables); } - // If we're allowed to write to the cache, update result.data to be - // the result as re-read from the cache, rather than the raw network - // result. Set without setDiff to avoid triggering a notify call, - // since we have other ways of notifying for this result. + // If we're allowed to write to the cache, and we can read a + // complete result from the cache, update result.data to be the + // result from the cache, rather than the raw network result. + // Set without setDiff to avoid triggering a notify call, since + // we have other ways of notifying for this result. this.updateLastDiff(diff, diffOptions); - result.data = diff.result; + if (diff.complete) { + result.data = diff.result; + } }); } else { this.lastWrite = void 0; } } - - return result; } public markReady() { diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index d97d8f23153..92029d9a6f1 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1194,7 +1194,7 @@ export class QueryManager { // Use linkDocument rather than queryInfo.document so the // operation/fragments used to write the result are the same as the // ones used to obtain it from the link. - result = queryInfo.markResult( + queryInfo.markResult( result, linkDocument, options, diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 9a958b7ccd8..49068a77548 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -67,6 +67,14 @@ export function resetStore(qm: QueryManager) { } describe("QueryManager", () => { + // Standard "get id from object" method. + const dataIdFromObject = (object: any) => { + if (object.__typename && object.id) { + return object.__typename + "__" + object.id; + } + return undefined; + }; + // Helper method that serves as the constructor method for // QueryManager but has defaults that make sense for these // tests. @@ -2217,6 +2225,107 @@ describe("QueryManager", () => { } ); + itAsync( + "should not return stale data when we orphan a real-id node in the store with a real-id node", + (resolve, reject) => { + const query1 = gql` + query { + author { + name { + firstName + lastName + } + age + id + __typename + } + } + `; + const query2 = gql` + query { + author { + name { + firstName + } + id + __typename + } + } + `; + const data1 = { + author: { + name: { + firstName: "John", + lastName: "Smith", + }, + age: 18, + id: "187", + __typename: "Author", + }, + }; + const data2 = { + author: { + name: { + firstName: "John", + }, + id: "197", + __typename: "Author", + }, + }; + const reducerConfig = { dataIdFromObject }; + const queryManager = createQueryManager({ + link: mockSingleLink( + { + request: { query: query1 }, + result: { data: data1 }, + }, + { + request: { query: query2 }, + result: { data: data2 }, + }, + { + request: { query: query1 }, + result: { data: data1 }, + } + ).setOnError(reject), + config: reducerConfig, + }); + + const observable1 = queryManager.watchQuery({ query: query1 }); + const observable2 = queryManager.watchQuery({ query: query2 }); + + // I'm not sure the waiting 60 here really is required, but the test used to do it + return Promise.all([ + observableToPromise( + { + observable: observable1, + wait: 60, + }, + (result) => { + expect(result).toEqual({ + data: data1, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + ), + observableToPromise( + { + observable: observable2, + wait: 60, + }, + (result) => { + expect(result).toEqual({ + data: data2, + loading: false, + networkStatus: NetworkStatus.ready, + }); + } + ), + ]).then(resolve, reject); + } + ); + itAsync( "should return partial data when configured when we orphan a real-id node in the store with a real-id node", (resolve, reject) => { @@ -2411,7 +2520,9 @@ describe("QueryManager", () => { loading: false, networkStatus: NetworkStatus.ready, data: { - info: {}, + info: { + a: "ay", + }, }, }); setTimeout(resolve, 100); diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 938781a9890..072ab6d922c 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -6017,10 +6017,7 @@ describe("useQuery Hook", () => { }, { interval: 1 } ); - const { vine, ...carDataWithoutVine } = carData.cars[0]; - expect(result.current.data).toEqual({ - cars: [carDataWithoutVine], - }); + expect(result.current.data).toEqual(carData); expect(result.current.error).toBeUndefined(); expect(consoleSpy.error).toHaveBeenCalled(); From cf11bb947826cb60da5cf39cde7c040f7d5cdec2 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Tue, 6 Feb 2024 15:39:56 -0500 Subject: [PATCH 2/4] chore: adds changeset --- .changeset/quick-pears-give.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quick-pears-give.md diff --git a/.changeset/quick-pears-give.md b/.changeset/quick-pears-give.md new file mode 100644 index 00000000000..aefe4d64e93 --- /dev/null +++ b/.changeset/quick-pears-give.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Revert PR [#11202](https://github.com/apollographql/apollo-client/pull/11202) to fix caching bug reported in [#11560](https://github.com/apollographql/apollo-client/issues/11560) From 79472c6f757cd8bdc32f292eaf8bb25d984f7ae3 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Tue, 6 Feb 2024 15:48:40 -0500 Subject: [PATCH 3/4] chore: update api reports --- .api-reports/api-report-core.md | 2 +- .api-reports/api-report-react.md | 2 +- .api-reports/api-report-react_components.md | 2 +- .api-reports/api-report-react_context.md | 2 +- .api-reports/api-report-react_hoc.md | 2 +- .api-reports/api-report-react_hooks.md | 2 +- .api-reports/api-report-react_internal.md | 2 +- .api-reports/api-report-react_ssr.md | 2 +- .api-reports/api-report-testing.md | 2 +- .api-reports/api-report-testing_core.md | 2 +- .api-reports/api-report-utilities.md | 2 +- .api-reports/api-report.md | 2 +- 12 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.api-reports/api-report-core.md b/.api-reports/api-report-core.md index 4d86ebc77c6..1ebf61607a3 100644 --- a/.api-reports/api-report-core.md +++ b/.api-reports/api-report-core.md @@ -1677,7 +1677,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react.md b/.api-reports/api-report-react.md index c695431d154..0cdf3ce93bc 100644 --- a/.api-reports/api-report-react.md +++ b/.api-reports/api-report-react.md @@ -1571,7 +1571,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_components.md b/.api-reports/api-report-react_components.md index 6b3d3fca46c..a4e212edb9f 100644 --- a/.api-reports/api-report-react_components.md +++ b/.api-reports/api-report-react_components.md @@ -1271,7 +1271,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_context.md b/.api-reports/api-report-react_context.md index 3d8e53adb6c..3ec73691a66 100644 --- a/.api-reports/api-report-react_context.md +++ b/.api-reports/api-report-react_context.md @@ -1202,7 +1202,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_hoc.md b/.api-reports/api-report-react_hoc.md index 15f30dae2df..107a19b3bbd 100644 --- a/.api-reports/api-report-react_hoc.md +++ b/.api-reports/api-report-react_hoc.md @@ -1247,7 +1247,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_hooks.md b/.api-reports/api-report-react_hooks.md index e1b188c020b..4abfa6b4bd0 100644 --- a/.api-reports/api-report-react_hooks.md +++ b/.api-reports/api-report-react_hooks.md @@ -1452,7 +1452,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_internal.md b/.api-reports/api-report-react_internal.md index 67cf910b346..0332db52334 100644 --- a/.api-reports/api-report-react_internal.md +++ b/.api-reports/api-report-react_internal.md @@ -1215,7 +1215,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-react_ssr.md b/.api-reports/api-report-react_ssr.md index 4bda11ff938..3747959ed85 100644 --- a/.api-reports/api-report-react_ssr.md +++ b/.api-reports/api-report-react_ssr.md @@ -1187,7 +1187,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-testing.md b/.api-reports/api-report-testing.md index 9be1fc3e0db..f57dfc0b93d 100644 --- a/.api-reports/api-report-testing.md +++ b/.api-reports/api-report-testing.md @@ -1264,7 +1264,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-testing_core.md b/.api-reports/api-report-testing_core.md index 0243870d739..bce86dc8645 100644 --- a/.api-reports/api-report-testing_core.md +++ b/.api-reports/api-report-testing_core.md @@ -1219,7 +1219,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report-utilities.md b/.api-reports/api-report-utilities.md index 24248cc6669..b47dd5550a0 100644 --- a/.api-reports/api-report-utilities.md +++ b/.api-reports/api-report-utilities.md @@ -2025,7 +2025,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) diff --git a/.api-reports/api-report.md b/.api-reports/api-report.md index 8774f31d096..1d09c28f12b 100644 --- a/.api-reports/api-report.md +++ b/.api-reports/api-report.md @@ -2138,7 +2138,7 @@ class QueryInfo { // Warning: (ae-forgotten-export) The symbol "CacheWriteBehavior" needs to be exported by the entry point index.d.ts // // (undocumented) - markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): typeof result; + markResult(result: FetchResult, document: DocumentNode, options: Pick, cacheWriteBehavior: CacheWriteBehavior): void; // (undocumented) networkError?: Error | null; // (undocumented) From e59d4be96432a8e9053cc678b5646bd5f6f136df Mon Sep 17 00:00:00 2001 From: alessbell Date: Tue, 6 Feb 2024 20:50:49 +0000 Subject: [PATCH 4/4] Clean up Prettier, Size-limit, and Api-Extractor --- .size-limits.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 07573418bb3..46dd4f15ae3 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39052, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32559 + "dist/apollo-client.min.cjs": 39043, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32550 }