Merge pull request apollographql#9823 from apollographql/issue-9765-l…

…eave-fetchPolicy-unchanged-when-skip-true

Leave `fetchPolicy` unchanged when `skip: true` (or in standby) and `nextFetchPolicy` is available

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Apollo Client 3.6.9 (unreleased)
+
+### Bug Fixes
+
+- Leave `fetchPolicy` unchanged when `skip: true` (or in standby) and `nextFetchPolicy` is available, even if `variables` change. <br/>
+  [@benjamn](https://github.com/benjamn) in [#9823](https://github.com/apollographql/apollo-client/pull/9823)
+
 ## Apollo Client 3.6.8 (2022-06-10)
 
 ### Bug Fixes

src/core/ObservableQuery.ts

@@ -653,18 +653,19 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
         initialFetchPolicy = fetchPolicy,
       } = options;
 
-      // When someone chooses "cache-and-network" or "network-only" as their
-      // 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. To that end, the
-      // options.nextFetchPolicy option provides an easy way to update
-      // options.fetchPolicy after the initial network request, without having to
-      // call observableQuery.setOptions.
-
-      if (typeof options.nextFetchPolicy === "function") {
+      if (fetchPolicy === "standby") {
+        // Do nothing, leaving options.fetchPolicy unchanged.
+      } else if (typeof options.nextFetchPolicy === "function") {
+        // When someone chooses "cache-and-network" or "network-only" as their
+        // 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. To
+        // that end, the options.nextFetchPolicy option provides an easy way to
+        // update options.fetchPolicy after the initial network request, without
+        // having to call observableQuery.setOptions.
         options.fetchPolicy = options.nextFetchPolicy(fetchPolicy, {
           reason,
           options,
@@ -809,7 +810,11 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
         newOptions &&
         newOptions.variables &&
         !equal(newOptions.variables, oldVariables) &&
-        (!newOptions.fetchPolicy || newOptions.fetchPolicy === oldFetchPolicy)
+        // Don't mess with the fetchPolicy if it's currently "standby".
+        options.fetchPolicy !== "standby" &&
+        // If we're changing the fetchPolicy anyway, don't try to change it here
+        // using applyNextFetchPolicy. The explicit options.fetchPolicy wins.
+        options.fetchPolicy === oldFetchPolicy
       ) {
         this.applyNextFetchPolicy("variables-changed", options);
         if (newNetworkStatus === void 0) {

src/core/QueryManager.ts

@@ -21,7 +21,7 @@ import {
   asyncMap,
   isNonEmptyArray,
   Concast,
-  ConcastSourcesIterable,
+  ConcastSourcesArray,
   makeUniqueId,
   isDocumentNode,
   isNonNullObject,
@@ -1122,16 +1122,33 @@ export class QueryManager<TStore> {
       // modify its properties here, rather than creating yet another new
       // WatchQueryOptions object.
       normalized.variables = variables;
-      return this.fetchQueryByPolicy<TData, TVars>(
+
+      const concastSources = this.fetchQueryByPolicy<TData, TVars>(
         queryInfo,
         normalized,
         networkStatus,
       );
+
+      if (
+        // If we're in standby, postpone advancing options.fetchPolicy using
+        // applyNextFetchPolicy.
+        normalized.fetchPolicy !== "standby" &&
+        // The "standby" policy currently returns [] from fetchQueryByPolicy, so
+        // this is another way to detect when nothing was done/fetched.
+        concastSources.length > 0 &&
+        queryInfo.observableQuery
+      ) {
+        queryInfo.observableQuery["applyNextFetchPolicy"]("after-fetch", options);
+      }
+
+      return concastSources;
     };
 
     // This cancel function needs to be set before the concast is created,
     // in case concast creation synchronously cancels the request.
+    const cleanupCancelFn = () => this.fetchCancelFns.delete(queryId);
     this.fetchCancelFns.set(queryId, reason => {
+      cleanupCancelFn();
       // This delay ensures the concast variable has been initialized.
       setTimeout(() => concast.cancel(reason));
     });
@@ -1156,13 +1173,7 @@ export class QueryManager<TStore> {
         : fromVariables(normalized.variables!)
     );
 
-    concast.cleanup(() => {
-      this.fetchCancelFns.delete(queryId);
-
-      if (queryInfo.observableQuery) {
-        queryInfo.observableQuery["applyNextFetchPolicy"]("after-fetch", options);
-      }
-    });
+    concast.promise.then(cleanupCancelFn, cleanupCancelFn);
 
     return concast;
   }
@@ -1338,7 +1349,7 @@ export class QueryManager<TStore> {
     // NetworkStatus.loading, but also possibly fetchMore, poll, refetch,
     // or setVariables.
     networkStatus: NetworkStatus,
-  ): ConcastSourcesIterable<ApolloQueryResult<TData>> {
+  ): ConcastSourcesArray<ApolloQueryResult<TData>> {
     const oldNetworkStatus = queryInfo.networkStatus;
 
     queryInfo.init({

src/core/__tests__/ObservableQuery.ts

@@ -2,7 +2,11 @@ import gql from 'graphql-tag';
 import { GraphQLError } from 'graphql';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 
-import { ApolloClient, NetworkStatus } from '../../core';
+import {
+  ApolloClient,
+  NetworkStatus,
+  WatchQueryFetchPolicy,
+} from '../../core';
 import { ObservableQuery } from '../ObservableQuery';
 import { QueryManager } from '../QueryManager';
 
@@ -1190,11 +1194,21 @@ describe('ObservableQuery', () => {
         },
       );
 
+      const usedFetchPolicies: WatchQueryFetchPolicy[] = [];
       const observable = queryManager.watchQuery({
         query: queryWithVars,
         variables: variables1,
         fetchPolicy: 'cache-and-network',
-        nextFetchPolicy: 'cache-first',
+        nextFetchPolicy(currentFetchPolicy, info) {
+          if (info.reason === "variables-changed") {
+            return info.initialFetchPolicy;
+          }
+          usedFetchPolicies.push(currentFetchPolicy);
+          if (info.reason === "after-fetch") {
+            return "cache-first";
+          }
+          return currentFetchPolicy;
+        },
         notifyOnNetworkStatusChange: true,
       });
 
@@ -1222,7 +1236,7 @@ describe('ObservableQuery', () => {
           }).then(result => {
             expect(result.data).toEqual(data);
           }).catch(reject);
-          expect(observable.options.fetchPolicy).toBe('cache-and-network');
+          expect(observable.options.fetchPolicy).toBe('cache-first');
         } else if (handleCount === 4) {
           expect(result.loading).toBe(true);
           expect(result.networkStatus).toBe(NetworkStatus.setVariables);
@@ -1236,7 +1250,7 @@ describe('ObservableQuery', () => {
           }).then(result => {
             expect(result.data).toEqual(data2);
           }).catch(reject);
-          expect(observable.options.fetchPolicy).toBe('cache-and-network');
+          expect(observable.options.fetchPolicy).toBe('cache-first');
         } else if (handleCount === 6) {
           expect(result.data).toEqual(data2);
           expect(result.loading).toBe(true);
@@ -1245,6 +1259,14 @@ describe('ObservableQuery', () => {
           expect(result.data).toEqual(data2);
           expect(result.loading).toBe(false);
           expect(observable.options.fetchPolicy).toBe('cache-first');
+
+          expect(usedFetchPolicies).toEqual([
+            "cache-and-network",
+            "network-only",
+            "cache-and-network",
+            "cache-and-network",
+          ]);
+
           setTimeout(resolve, 10);
         } else {
           reject(`too many renders (${handleCount})`);

src/core/__tests__/fetchPolicies.ts

@@ -896,7 +896,7 @@ describe("nextFetchPolicy", () => {
       }).catch(reject);
 
       // Changing variables resets the fetchPolicy to its initial value.
-      expect(observable.options.fetchPolicy).toBe("network-only");
+      expect(observable.options.fetchPolicy).toBe("cache-first");
 
     } else if (count === 3) {
       expect(result.loading).toBe(false);

src/react/hoc/__tests__/queries/loading.test.tsx

@@ -3,7 +3,7 @@ import { render, waitFor } from '@testing-library/react';
 import gql from 'graphql-tag';
 import { DocumentNode } from 'graphql';
 
-import { ApolloClient, NetworkStatus } from '../../../../core';
+import { ApolloClient, NetworkStatus, WatchQueryFetchPolicy } from '../../../../core';
 import { ApolloProvider } from '../../../context';
 import { InMemoryCache as Cache } from '../../../../cache';
 import { itAsync, mockSingleLink } from '../../../../testing';
@@ -360,32 +360,46 @@ describe('[queries] loading', () => {
 
     let count = 0;
 
+    const usedFetchPolicies: WatchQueryFetchPolicy[] = [];
     const Container = graphql<{}, Data>(query, {
       options: {
         fetchPolicy: 'network-only',
-        nextFetchPolicy: 'cache-first',
+        nextFetchPolicy(currentFetchPolicy, info) {
+          if (info.reason === "variables-changed") {
+            return info.initialFetchPolicy;
+          }
+          usedFetchPolicies.push(currentFetchPolicy);
+          if (info.reason === "after-fetch") {
+            return "cache-first";
+          }
+          return currentFetchPolicy;
+        }
       },
     })(
       class extends React.Component<ChildProps<{}, Data>> {
         render() {
+          ++count;
           if (count === 1) {
+            expect(this.props.data!.loading).toBe(true);
+            expect(this.props.data!.allPeople).toBeUndefined();
+          } else if (count === 2) {
+            expect(this.props.data!.loading).toBe(false);
+            expect(this.props.data!.allPeople!.people[0].name).toMatch(
+              /Darth Skywalker - /
+            );
             // Has data
-            setTimeout(() => {
-              render(App);
-            }, 0);
-          }
-          if (count === 2) {
+            setTimeout(() => render(App));
+          } else if (count === 3) {
             // Loading after remount
-            expect(this.props.data!.loading).toBeTruthy();
-          }
-          if (count === 3) {
+            expect(this.props.data!.loading).toBe(true);
+            expect(this.props.data!.allPeople).toBeUndefined();
+          } else if (count >= 4) {
             // Fetched data loading after remount
-            expect(this.props.data!.loading).toBeFalsy();
+            expect(this.props.data!.loading).toBe(false);
             expect(this.props.data!.allPeople!.people[0].name).toMatch(
               /Darth Skywalker - /
             );
           }
-          count += 1;
           return null;
         }
       }
@@ -399,7 +413,14 @@ describe('[queries] loading', () => {
 
     render(App);
 
-    waitFor(() => expect(count).toBe(5)).then(resolve, reject);
+    return waitFor(() => {
+      expect(usedFetchPolicies).toEqual([
+        "network-only",
+        "network-only",
+        "cache-first",
+      ]);
+      expect(count).toBe(6);
+    }).then(resolve, reject);
   });
 
   itAsync('correctly sets loading state on remounted component with changed variables', (resolve, reject) => {

src/react/hoc/__tests__/queries/skip.test.tsx

@@ -670,7 +670,7 @@ describe('[queries] skip', () => {
                 break;
               case 4:
                 expect(this.props.skip).toBe(false);
-                expect(this.props.data!.loading).toBe(true);
+                expect(this.props.data!.loading).toBe(false);
                 expect(this.props.data.allPeople).toEqual(data.allPeople);
                 expect(ranQuery).toBe(2);
                 break;

src/react/hooks/__tests__/useQuery.test.tsx

@@ -1037,7 +1037,7 @@ describe('useQuery Hook', () => {
 
       expect(
         result.current.observable.options.fetchPolicy
-      ).toBe("network-only");
+      ).toBe("cache-first");
 
       expect(result.current.observable.variables).toEqual({
         sourceOfVar: "reobserve",
@@ -1094,7 +1094,7 @@ describe('useQuery Hook', () => {
 
       expect(
         result.current.observable.options.fetchPolicy
-      ).toBe("network-only");
+      ).toBe("cache-first");
 
       expect(result.current.observable.variables).toEqual({
         sourceOfVar: "reobserve without variable merge",

src/utilities/observables/Concast.ts

@@ -12,6 +12,7 @@ function isPromiseLike<T>(value: MaybeAsync<T>): value is PromiseLike<T> {
 type Source<T> = MaybeAsync<Observable<T>>;
 
 export type ConcastSourcesIterable<T> = Iterable<Source<T>>;
+export type ConcastSourcesArray<T> = Array<Source<T>>;
 
 // A Concast<T> observable concatenates the given sources into a single
 // non-overlapping sequence of Ts, automatically unwrapping any promises,