diff --git a/.changeset/shaggy-ears-scream.md b/.changeset/shaggy-ears-scream.md new file mode 100644 index 00000000000..3ec33bfab58 --- /dev/null +++ b/.changeset/shaggy-ears-scream.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": minor +--- + +Prevent `QueryInfo#markResult` mutation of `result.data` and return cache data consistently whether complete or incomplete. diff --git a/.size-limit.cjs b/.size-limit.cjs index 096d4ebcf85..064303a8ea8 100644 --- a/.size-limit.cjs +++ b/.size-limit.cjs @@ -10,7 +10,7 @@ const checks = [ { path: "dist/index.js", import: "{ ApolloClient, InMemoryCache, HttpLink }", - limit: "32044", + limit: "32052", }, ...[ "ApolloProvider", diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index c89098362d4..9df645767c7 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -2921,7 +2921,10 @@ describe("client", () => { return client .query({ query }) .then(({ data }) => { - expect(data).toEqual(result.data); + const { price, ...todoWithoutPrice } = data.todos[0]; + expect(data).toEqual({ + todos: [todoWithoutPrice], + }); }) .then(resolve, reject); } diff --git a/src/cache/inmemory/__tests__/client.ts b/src/cache/inmemory/__tests__/client.ts new file mode 100644 index 00000000000..c3844cb20c0 --- /dev/null +++ b/src/cache/inmemory/__tests__/client.ts @@ -0,0 +1,171 @@ +// 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 4ddb60fce34..f666456160c 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -361,7 +361,8 @@ 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) @@ -408,7 +409,10 @@ export class QueryInfo { }); this.lastWrite = { - result, + // 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 }, variables: options.variables, dmCount: destructiveMethodCounts.get(this.cache), }; @@ -470,20 +474,19 @@ export class QueryInfo { this.updateWatch(options.variables); } - // 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. + // 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. this.updateLastDiff(diff, diffOptions); - if (diff.complete) { - result.data = diff.result; - } + 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 efdd3c05a92..15f81132194 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1176,7 +1176,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. - queryInfo.markResult( + result = queryInfo.markResult( result, linkDocument, options, diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 77fd0fe6dbd..e1eeb02893c 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -66,14 +66,6 @@ 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. @@ -2224,107 +2216,6 @@ 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) => { @@ -2519,9 +2410,7 @@ describe("QueryManager", () => { loading: false, networkStatus: NetworkStatus.ready, data: { - info: { - a: "ay", - }, + info: {}, }, }); setTimeout(resolve, 100); diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 96810b6414e..3cd7a0158e7 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -5706,7 +5706,10 @@ describe("useQuery Hook", () => { }, { interval: 1 } ); - expect(result.current.data).toEqual(carData); + const { vine, ...carDataWithoutVine } = carData.cars[0]; + expect(result.current.data).toEqual({ + cars: [carDataWithoutVine], + }); expect(result.current.error).toBeUndefined(); expect(errorSpy).toHaveBeenCalled();