diff --git a/.changeset/nice-worms-juggle.md b/.changeset/nice-worms-juggle.md new file mode 100644 index 00000000000..92aa1051b69 --- /dev/null +++ b/.changeset/nice-worms-juggle.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where `fetchMore` would write its result data to the cache when using it with a `no-cache` fetch policy. diff --git a/.changeset/pink-horses-pay.md b/.changeset/pink-horses-pay.md new file mode 100644 index 00000000000..c2773153ddf --- /dev/null +++ b/.changeset/pink-horses-pay.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where executing `fetchMore` with a `no-cache` fetch policy could sometimes result in multiple network requests. diff --git a/.changeset/short-scissors-speak.md b/.changeset/short-scissors-speak.md new file mode 100644 index 00000000000..38a6d8aa45b --- /dev/null +++ b/.changeset/short-scissors-speak.md @@ -0,0 +1,7 @@ +--- +"@apollo/client": patch +--- + +**Potentially disruptive change** + +When calling `fetchMore` with a query that has a `no-cache` fetch policy, `fetchMore` will now throw if an `updateQuery` function is not provided. This provides a mechanism to merge the results from the `fetchMore` call with the query's previous result. diff --git a/.size-limits.json b/.size-limits.json index 6f43705ee6e..0e52c9ae9c7 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 40168, - "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32974 + "dist/apollo-client.min.cjs": 40243, + "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 33041 } diff --git a/docs/source/pagination/core-api.mdx b/docs/source/pagination/core-api.mdx index 6d4f0b0b0f3..d1aee410565 100644 --- a/docs/source/pagination/core-api.mdx +++ b/docs/source/pagination/core-api.mdx @@ -199,7 +199,7 @@ const cache = new InMemoryCache({ Query: { fields: { feed: { - keyArgs: [], + keyArgs: false, merge(existing, incoming, { args: { offset = 0 }}) { // Slicing is necessary because the existing data is // immutable, and frozen in development. @@ -218,6 +218,41 @@ const cache = new InMemoryCache({ This logic handles sequential page writes the same way the single-line strategy does, but it can also tolerate repeated, overlapping, or out-of-order writes, without duplicating any list items. +### Updating the query with the fetch more result + +At times the call to `fetchMore` may need to perform additional cache updates for your query. While you can use the [`cache.readQuery`](/caching/cache-interaction#readquery) and [`cache.writeQuery`](/caching/cache-interaction#writequery) functions to to do this work yourself, it can be cumbsersome to use both of these together. + +As a shortcut, you can provide the `updateQuery` option to `fetchMore` to update your query using the result from the `fetchMore` call. + + + +`updateQuery` is not a replacement for your field policy `merge` functions. While you can use `updateQuery` without the need to define a `merge` function, `merge` functions defined for fields in the query will run using the result from `updateQuery`. + + + +Let's see the example above using `updateQuery` to merge results together instead of a field policy merge function: + +```ts +fetchMore({ + variables: { offset: data.feed.length }, + updateQuery(previousData, { fetchMoreResult, variables: { offset }}) { + // Slicing is necessary because the existing data is + // immutable, and frozen in development. + const updatedFeed = previousData.feed.slice(0); + for (let i = 0; i < fetchMoreResult.feed.length; ++i) { + updatedFeed[offset + i] = fetchMoreResult.feed[i]; + } + return { ...previousData, feed: updatedFeed }; + }, +}) +``` + + + +We recommend defining field policies that contain at least a [`keyArgs`](/pagination/key-args/) value even when you use `updateQuery`. This prevents fragmenting the data unnecessarily in the cache. Setting `keyArgs` to `false` is adequate for most situations to ignore the `offset` and `limit` arguments and write the paginated data as one big array. + + + ## `read` functions for paginated fields [As shown above](#defining-a-field-policy), a `merge` function helps you combine paginated query results from your GraphQL server into a single list in your client cache. But what if you also want to configure how that locally cached list is _read_? For that, you can define a **`read` function**. @@ -304,3 +339,38 @@ The `read` function for a paginated field can choose to _ignore_ arguments like If you adopt this approach, you might not need to define a `read` function at all, because the cached list can be returned without any processing. That's why the [`offsetLimitPagination` helper function](./offset-based/#the-offsetlimitpagination-helper) is implemented _without_ a `read` function. Regardless of how you configure `keyArgs`, your `read` (and `merge`) functions can always examine any arguments passed to the field using the `options.args` object. See [The `keyArgs` API](./key-args) for a deeper discussion of how to reason about dividing argument-handling responsibility between `keyArgs` and your `read` or `merge` functions. + +## Using `fetchMore` with queries that set a `no-cache` fetch policy + + + +We recommend upgrading to version 3.11.3 or later to address bugs that exhibit unexpected behavior when using `fetchMore` with queries that set `no-cache` fetch policies. Please see pull request [#11974](https://github.com/apollographql/apollo-client/pull/11974) for more information. + + + +The examples shown above use field policies and `merge` functions to update the result of a paginated field. But what about queries that use a `no-cache` fetch policy? Data is not written to the cache, so field policies have no effect. + +To update our query, we provide the `updateQuery` option to the `fetchMore` function. + +Let's use the example above, but instead provide the `updateQuery` function to `fetchMore` to update the query. + +```ts +fetchMore({ + variables: { offset: data.feed.length }, + updateQuery(previousData, { fetchMoreResult, variables: { offset }}) { + // Slicing is necessary because the existing data is + // immutable, and frozen in development. + const updatedFeed = previousData.feed.slice(0); + for (let i = 0; i < fetchMoreResult.feed.length; ++i) { + updatedFeed[offset + i] = fetchMoreResult.feed[i]; + } + return { ...previousData, feed: updatedFeed }; + }, +}) +``` + + + +As of Apollo Client version 3.11.3, the `updateQuery` option is required when using `fetchMore` with a `no-cache` fetch policy. This is required to correctly determine how the results should be merged since field policy `merge` functions are ignored. Calling `fetchMore` without an `updateQuery` function will throw an error. + + diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 7a419ff078e..1289a9735ef 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -484,6 +484,16 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`, const updatedQuerySet = new Set(); + const updateQuery = fetchMoreOptions?.updateQuery; + const isCached = this.options.fetchPolicy !== "no-cache"; + + if (!isCached) { + invariant( + updateQuery, + "You must provide an `updateQuery` function when using `fetchMore` with a `no-cache` fetch policy." + ); + } + return this.queryManager .fetchQuery(qid, combinedOptions, NetworkStatus.fetchMore) .then((fetchMoreResult) => { @@ -493,48 +503,72 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`, queryInfo.networkStatus = originalNetworkStatus; } - // Performing this cache update inside a cache.batch transaction ensures - // any affected cache.watch watchers are notified at most once about any - // updates. Most watchers will be using the QueryInfo class, which - // responds to notifications by calling reobserveCacheFirst to deliver - // fetchMore cache results back to this ObservableQuery. - this.queryManager.cache.batch({ - update: (cache) => { - const { updateQuery } = fetchMoreOptions; - if (updateQuery) { - cache.updateQuery( - { - query: this.query, - variables: this.variables, - returnPartialData: true, - optimistic: false, - }, - (previous) => - updateQuery(previous!, { - fetchMoreResult: fetchMoreResult.data, - variables: combinedOptions.variables as TFetchVars, - }) - ); - } else { - // If we're using a field policy instead of updateQuery, the only - // thing we need to do is write the new data to the cache using - // combinedOptions.variables (instead of this.variables, which is - // what this.updateQuery uses, because it works by abusing the - // original field value, keyed by the original variables). - cache.writeQuery({ - query: combinedOptions.query, - variables: combinedOptions.variables, - data: fetchMoreResult.data, - }); - } - }, + if (isCached) { + // Performing this cache update inside a cache.batch transaction ensures + // any affected cache.watch watchers are notified at most once about any + // updates. Most watchers will be using the QueryInfo class, which + // responds to notifications by calling reobserveCacheFirst to deliver + // fetchMore cache results back to this ObservableQuery. + this.queryManager.cache.batch({ + update: (cache) => { + const { updateQuery } = fetchMoreOptions; + if (updateQuery) { + cache.updateQuery( + { + query: this.query, + variables: this.variables, + returnPartialData: true, + optimistic: false, + }, + (previous) => + updateQuery(previous!, { + fetchMoreResult: fetchMoreResult.data, + variables: combinedOptions.variables as TFetchVars, + }) + ); + } else { + // If we're using a field policy instead of updateQuery, the only + // thing we need to do is write the new data to the cache using + // combinedOptions.variables (instead of this.variables, which is + // what this.updateQuery uses, because it works by abusing the + // original field value, keyed by the original variables). + cache.writeQuery({ + query: combinedOptions.query, + variables: combinedOptions.variables, + data: fetchMoreResult.data, + }); + } + }, - onWatchUpdated: (watch) => { - // Record the DocumentNode associated with any watched query whose - // data were updated by the cache writes above. - updatedQuerySet.add(watch.query); - }, - }); + onWatchUpdated: (watch) => { + // Record the DocumentNode associated with any watched query whose + // data were updated by the cache writes above. + updatedQuerySet.add(watch.query); + }, + }); + } else { + // There is a possibility `lastResult` may not be set when + // `fetchMore` is called which would cause this to crash. This should + // only happen if we haven't previously reported a result. We don't + // quite know what the right behavior should be here since this block + // of code runs after the fetch result has executed on the network. + // We plan to let it crash in the meantime. + // + // If we get bug reports due to the `data` property access on + // undefined, this should give us a real-world scenario that we can + // use to test against and determine the right behavior. If we do end + // up changing this behavior, this may require, for example, an + // adjustment to the types on `updateQuery` since that function + // expects that the first argument always contains previous result + // data, but not `undefined`. + const lastResult = this.getLast("result")!; + const data = updateQuery!(lastResult.data, { + fetchMoreResult: fetchMoreResult.data, + variables: combinedOptions.variables as TFetchVars, + }); + + this.reportResult({ ...lastResult, data }, this.variables); + } return fetchMoreResult; }) @@ -544,7 +578,7 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`, // likely because the written data were the same as what was already in // the cache, we still want fetchMore to deliver its final loading:false // result with the unchanged data. - if (!updatedQuerySet.has(this.query)) { + if (isCached && !updatedQuerySet.has(this.query)) { reobserveCacheFirst(this); } }); diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 19a1ba57687..7c87e28a92f 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -7,6 +7,7 @@ import { render, screen, waitFor, renderHook } from "@testing-library/react"; import { ApolloClient, ApolloError, + ApolloQueryResult, NetworkStatus, OperationVariables, TypedDocumentNode, @@ -32,12 +33,15 @@ import { useMutation } from "../useMutation"; import { createProfiler, disableActWarnings, + PaginatedCaseData, profileHook, + setupPaginatedCase, spyOnConsole, } from "../../../testing/internal"; import { useApolloClient } from "../useApolloClient"; import { useLazyQuery } from "../useLazyQuery"; import { mockFetchQuery } from "../../../core/__tests__/ObservableQuery"; +import { InvariantError } from "../../../utilities/globals"; const IS_REACT_17 = React.version.startsWith("17"); @@ -4049,6 +4053,297 @@ describe("useQuery Hook", () => { expect(result.current.data).toEqual({ letters: ab.concat(cd) }); }); + // https://github.com/apollographql/apollo-client/issues/11965 + it("should only execute single network request when calling fetchMore with no-cache fetch policy", async () => { + let fetches: Array<{ variables: Record }> = []; + const { query, data } = setupPaginatedCase(); + + const link = new ApolloLink((operation) => { + fetches.push({ variables: operation.variables }); + + const { offset = 0, limit = 2 } = operation.variables; + const letters = data.slice(offset, offset + limit); + + return new Observable((observer) => { + setTimeout(() => { + observer.next({ data: { letters } }); + observer.complete(); + }, 10); + }); + }); + + const client = new ApolloClient({ cache: new InMemoryCache(), link }); + + const ProfiledHook = profileHook(() => + useQuery(query, { fetchPolicy: "no-cache", variables: { limit: 2 } }) + ); + + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + // loading + await ProfiledHook.takeSnapshot(); + // finished loading + await ProfiledHook.takeSnapshot(); + + expect(fetches).toStrictEqual([{ variables: { limit: 2 } }]); + + const { fetchMore } = ProfiledHook.getCurrentSnapshot(); + + await act(() => + fetchMore({ + variables: { offset: 2 }, + updateQuery: (_, { fetchMoreResult }) => fetchMoreResult, + }) + ); + + expect(fetches).toStrictEqual([ + { variables: { limit: 2 } }, + { variables: { limit: 2, offset: 2 } }, + ]); + }); + + it("uses updateQuery to update the result of the query with no-cache queries", async () => { + const { query, link } = setupPaginatedCase(); + + const client = new ApolloClient({ cache: new InMemoryCache(), link }); + + const ProfiledHook = profileHook(() => + useQuery(query, { + notifyOnNetworkStatusChange: true, + fetchPolicy: "no-cache", + variables: { limit: 2 }, + }) + ); + + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + { + const { loading, networkStatus, data } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(true); + expect(networkStatus).toBe(NetworkStatus.loading); + expect(data).toBeUndefined(); + } + + { + const { loading, networkStatus, data } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(false); + expect(networkStatus).toBe(NetworkStatus.ready); + expect(data).toStrictEqual({ + letters: [ + { __typename: "Letter", letter: "A", position: 1 }, + { __typename: "Letter", letter: "B", position: 2 }, + ], + }); + } + + let fetchMorePromise!: Promise>; + const { fetchMore } = ProfiledHook.getCurrentSnapshot(); + + act(() => { + fetchMorePromise = fetchMore({ + variables: { offset: 2 }, + updateQuery: (prev, { fetchMoreResult }) => ({ + letters: prev.letters.concat(fetchMoreResult.letters), + }), + }); + }); + + { + const { loading, networkStatus, data } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(true); + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect(data).toStrictEqual({ + letters: [ + { __typename: "Letter", letter: "A", position: 1 }, + { __typename: "Letter", letter: "B", position: 2 }, + ], + }); + } + + { + const { loading, networkStatus, data, observable } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(false); + expect(networkStatus).toBe(NetworkStatus.ready); + expect(data).toEqual({ + letters: [ + { __typename: "Letter", letter: "A", position: 1 }, + { __typename: "Letter", letter: "B", position: 2 }, + { __typename: "Letter", letter: "C", position: 3 }, + { __typename: "Letter", letter: "D", position: 4 }, + ], + }); + + // Ensure we store the merged result as the last result + expect(observable.getCurrentResult(false).data).toEqual({ + letters: [ + { __typename: "Letter", letter: "A", position: 1 }, + { __typename: "Letter", letter: "B", position: 2 }, + { __typename: "Letter", letter: "C", position: 3 }, + { __typename: "Letter", letter: "D", position: 4 }, + ], + }); + } + + await expect(fetchMorePromise).resolves.toStrictEqual({ + data: { + letters: [ + { __typename: "Letter", letter: "C", position: 3 }, + { __typename: "Letter", letter: "D", position: 4 }, + ], + }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + + await expect(ProfiledHook).not.toRerender(); + + act(() => { + fetchMorePromise = fetchMore({ + variables: { offset: 4 }, + updateQuery: (_, { fetchMoreResult }) => fetchMoreResult, + }); + }); + + { + const { data, loading, networkStatus } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(true); + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect(data).toEqual({ + letters: [ + { __typename: "Letter", letter: "A", position: 1 }, + { __typename: "Letter", letter: "B", position: 2 }, + { __typename: "Letter", letter: "C", position: 3 }, + { __typename: "Letter", letter: "D", position: 4 }, + ], + }); + } + + { + const { data, loading, networkStatus, observable } = + await ProfiledHook.takeSnapshot(); + + expect(loading).toBe(false); + expect(networkStatus).toBe(NetworkStatus.ready); + expect(data).toEqual({ + letters: [ + { __typename: "Letter", letter: "E", position: 5 }, + { __typename: "Letter", letter: "F", position: 6 }, + ], + }); + + expect(observable.getCurrentResult(false).data).toEqual({ + letters: [ + { __typename: "Letter", letter: "E", position: 5 }, + { __typename: "Letter", letter: "F", position: 6 }, + ], + }); + } + + await expect(fetchMorePromise).resolves.toStrictEqual({ + data: { + letters: [ + { __typename: "Letter", letter: "E", position: 5 }, + { __typename: "Letter", letter: "F", position: 6 }, + ], + }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + + await expect(ProfiledHook).not.toRerender(); + }); + + it("throws when using fetchMore without updateQuery for no-cache queries", async () => { + const { query, link } = setupPaginatedCase(); + + const client = new ApolloClient({ cache: new InMemoryCache(), link }); + + const ProfiledHook = profileHook(() => + useQuery(query, { fetchPolicy: "no-cache", variables: { limit: 2 } }) + ); + + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + // loading + await ProfiledHook.takeSnapshot(); + // finished loading + await ProfiledHook.takeSnapshot(); + + const { fetchMore } = ProfiledHook.getCurrentSnapshot(); + + expect(() => fetchMore({ variables: { offset: 2 } })).toThrow( + new InvariantError( + "You must provide an `updateQuery` function when using `fetchMore` with a `no-cache` fetch policy." + ) + ); + }); + + it("does not write to cache when using fetchMore with no-cache queries", async () => { + const { query, data } = setupPaginatedCase(); + + const link = new ApolloLink((operation) => { + const { offset = 0, limit = 2 } = operation.variables; + const letters = data.slice(offset, offset + limit); + + return new Observable((observer) => { + setTimeout(() => { + observer.next({ data: { letters } }); + observer.complete(); + }, 10); + }); + }); + + const client = new ApolloClient({ cache: new InMemoryCache(), link }); + + const ProfiledHook = profileHook(() => + useQuery(query, { fetchPolicy: "no-cache", variables: { limit: 2 } }) + ); + + render(, { + wrapper: ({ children }) => ( + {children} + ), + }); + + // initial loading + await ProfiledHook.takeSnapshot(); + + // Initial result + await ProfiledHook.takeSnapshot(); + + const { fetchMore } = ProfiledHook.getCurrentSnapshot(); + await act(() => + fetchMore({ + variables: { offset: 2 }, + updateQuery: (_, { fetchMoreResult }) => fetchMoreResult, + }) + ); + + expect(client.extract()).toStrictEqual({}); + }); + it("regression test for issue #8600", async () => { const cache = new InMemoryCache({ typePolicies: { diff --git a/src/testing/internal/scenarios/index.ts b/src/testing/internal/scenarios/index.ts index 2ed2bb5c523..c6943f51236 100644 --- a/src/testing/internal/scenarios/index.ts +++ b/src/testing/internal/scenarios/index.ts @@ -106,5 +106,5 @@ export function setupPaginatedCase() { }); }); - return { query, link }; + return { query, link, data }; }