From 222b2a461764f14f7b583e977174690e33fcb0e0 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 27 Jul 2020 15:21:46 -0400 Subject: [PATCH] Revert PR #6353 and support options.nextFetchPolicy instead. PR #6353 seemed like a clever zero-configuration way to improve the default behavior of the cache-and-network and network-only fetch policies. Even though the word "network" is right there in the policy strings, it can be surprising (see #6305) to see network requests happening when you didn't expect them. However, the wisdom of #6353 was contingent on this new behavior of falling back to cache-first not creating problems for anyone, and unfortunately that hope appears to have been overly optimistic/naive: https://github.com/apollographql/apollo-client/issues/6677#issuecomment-664212051 To keep everyone happy, I think we need to revert #6353 while providing an easy way to achieve the same effect, when that's what you want. The new options.nextFetchPolicy option allows updating options.fetchPolicy after the intial network request, without calling observableQuery.setOptions. This could be considered a breaking change, but it's also a regression from Apollo Client 2.x that needs fixing. We are confident options.nextFetchPolicy will restore the #6353 functionality where desired, and we are much more comfortable requiring the explicit use of options.nextFetchPolicy in the future. --- src/core/ObservableQuery.ts | 21 ++++++++------ src/core/QueryManager.ts | 28 +++++++++---------- src/core/__tests__/ObservableQuery.ts | 18 ++++++++---- src/core/watchQueryOptions.ts | 6 +++- .../hoc/__tests__/queries/loading.test.tsx | 5 +++- src/react/hoc/__tests__/queries/skip.test.tsx | 1 + src/react/types/types.ts | 1 + 7 files changed, 50 insertions(+), 30 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 7bff619a1ae..e880d8c1277 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -219,28 +219,33 @@ export class ObservableQuery< )); } + const reobserveOptions: Partial> = { + // Always disable polling for refetches. + pollInterval: 0, + }; + // Unless the provided fetchPolicy always consults the network // (no-cache, network-only, or cache-and-network), override it with // network-only to force the refetch for this fetchQuery call. if (fetchPolicy !== 'no-cache' && fetchPolicy !== 'cache-and-network') { - fetchPolicy = 'network-only'; + reobserveOptions.fetchPolicy = 'network-only'; + // Go back to the original options.fetchPolicy after this refetch. + reobserveOptions.nextFetchPolicy = fetchPolicy; } if (variables && !equal(this.options.variables, variables)) { // Update the existing options with new variables - this.options.variables = { + reobserveOptions.variables = this.options.variables = { ...this.options.variables, ...variables, } as TVariables; } - return this.newReobserver(false).reobserve({ - fetchPolicy, - variables: this.options.variables, - // Always disable polling for refetches. - pollInterval: 0, - }, NetworkStatus.refetch); + return this.newReobserver(false).reobserve( + reobserveOptions, + NetworkStatus.refetch, + ); } public fetchMore( diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index da6fc4b80a6..6966570b9d1 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -909,22 +909,20 @@ export class QueryManager { concast.cleanup(() => { this.fetchCancelFns.delete(queryId); - if (fetchPolicy === "cache-and-network" || - fetchPolicy === "network-only") { + if (options.nextFetchPolicy) { // When someone chooses cache-and-network or network-only as their - // initial FetchPolicy, they almost certainly do not want future cache - // updates to trigger unconditional network requests, which is what - // repeatedly applying the cache-and-network or network-only policies - // would seem to require. Instead, when the cache reports an update - // after the initial network request, subsequent network requests should - // be triggered only if the cache result is incomplete. This behavior - // corresponds exactly to switching to a cache-first FetchPolicy, so we - // modify options.fetchPolicy here for the next fetchQueryObservable - // call, using the same options object that the Reobserver always passes - // to fetchQueryObservable. Note: if these FetchPolicy transitions get - // much more complicated, we might consider using some sort of state - // machine to capture the transition rules. - options.fetchPolicy = "cache-first"; + // initial FetchPolicy, they often do not want future cache updates to + // trigger unconditional network requests, which is what repeatedly + // applying the cache-and-network or network-only policies would seem + // to imply. Instead, when the cache reports an update after the + // initial network request, it may be desirable for subsequent network + // requests to be triggered only if the cache result is incomplete. + // The options.nextFetchPolicy option provides an easy way to update + // options.fetchPolicy after the intial network request, without + // having to call observableQuery.setOptions. + options.fetchPolicy = options.nextFetchPolicy; + // The options.nextFetchPolicy transition should happen only once. + options.nextFetchPolicy = void 0; } }); diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 4ecca7bf4da..d2ec4b1d356 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -986,8 +986,7 @@ describe('ObservableQuery', () => { // Although the options.fetchPolicy we passed just now to // fetchQueryByPolicy should have been network-only, // observable.options.fetchPolicy should now be updated to - // cache-first, since network-only (and cache-and-network) fetch - // policies fall back to cache-first after the first request. + // cache-first, thanks to options.nextFetchPolicy. expect(observable.options.fetchPolicy).toBe('cache-first'); const fqoCalls = mocks.fetchQueryObservable.mock.calls; expect(fqoCalls.length).toBe(2); @@ -1206,17 +1205,26 @@ describe('ObservableQuery', () => { observable.refetch().then(result => { expect(result).toEqual({ data: { - counter: 3, + counter: 5, name: 'Ben', }, loading: false, networkStatus: NetworkStatus.ready, }); - resolve(); + setTimeout(resolve, 50); }, reject); }, ); - } else if (handleCount > 2) { + } else if (handleCount === 3) { + expect(result).toEqual({ + data: { + counter: 3, + name: 'Ben', + }, + loading: true, + networkStatus: NetworkStatus.refetch, + }); + } else if (handleCount > 3) { reject(new Error('should not get here')); } }, diff --git a/src/core/watchQueryOptions.ts b/src/core/watchQueryOptions.ts index fe598bac9be..d865367214f 100644 --- a/src/core/watchQueryOptions.ts +++ b/src/core/watchQueryOptions.ts @@ -108,9 +108,13 @@ export interface WatchQueryOptions extends QueryBaseOptions, ModifiableWatchQueryOptions { /** - * Specifies the {@link FetchPolicy} to be used for this query + * Specifies the {@link FetchPolicy} to be used for this query. */ fetchPolicy?: WatchQueryFetchPolicy; + /** + * Specifies the {@link FetchPolicy} to be used after this query has completed. + */ + nextFetchPolicy?: WatchQueryFetchPolicy; } export interface FetchMoreQueryOptions { diff --git a/src/react/hoc/__tests__/queries/loading.test.tsx b/src/react/hoc/__tests__/queries/loading.test.tsx index 3289f58c216..9c0bab58731 100644 --- a/src/react/hoc/__tests__/queries/loading.test.tsx +++ b/src/react/hoc/__tests__/queries/loading.test.tsx @@ -328,7 +328,10 @@ describe('[queries] loading', () => { let count = 0; const Container = graphql<{}, Data>(query, { - options: { fetchPolicy: 'network-only' } + options: { + fetchPolicy: 'network-only', + nextFetchPolicy: 'cache-first', + }, })( class extends React.Component> { render() { diff --git a/src/react/hoc/__tests__/queries/skip.test.tsx b/src/react/hoc/__tests__/queries/skip.test.tsx index e80c842c046..9f254236e74 100644 --- a/src/react/hoc/__tests__/queries/skip.test.tsx +++ b/src/react/hoc/__tests__/queries/skip.test.tsx @@ -613,6 +613,7 @@ describe('[queries] skip', () => { const Container = graphql(query, { options: { fetchPolicy: 'network-only', + nextFetchPolicy: 'cache-first', notifyOnNetworkStatusChange: true }, skip: ({ skip }) => skip diff --git a/src/react/types/types.ts b/src/react/types/types.ts index ed828dd319c..cfba43611df 100644 --- a/src/react/types/types.ts +++ b/src/react/types/types.ts @@ -33,6 +33,7 @@ export interface BaseQueryOptions { ssr?: boolean; variables?: TVariables; fetchPolicy?: WatchQueryFetchPolicy; + nextFetchPolicy?: WatchQueryFetchPolicy; errorPolicy?: ErrorPolicy; pollInterval?: number; client?: ApolloClient;