Merge pull request #8465 from apollographql/ObservableQuery-nextFetch…

…Policy-improvements

Improve processing of options.nextFetchPolicy

CHANGELOG.md

@@ -56,6 +56,9 @@
 - Log non-fatal `invariant.error` message when fields are missing from result objects written into `InMemoryCache`, rather than throwing an exception. While this change relaxes an exception to be merely an error message, which is usually a backwards-compatible change, the error messages are logged in more cases now than the exception was previously thrown, and those new error messages may be worth investigating to discover potential problems in your application. The errors are not displayed for `@client`-only fields, so adding `@client` is one way to handle/hide the errors for local-only fields. Another general strategy is to use a more precise query to write specific subsets of data into the cache, rather than reusing a larger query that contains fields not present in the written `data`. <br/>
   [@benjamn](https://github.com/benjamn) in [#8416](https://github.com/apollographql/apollo-client/pull/8416)
 
+- The [`nextFetchPolicy`](https://github.com/apollographql/apollo-client/pull/6893) option for `client.watchQuery` and `useQuery` will no longer be removed from the `options` object after it has been applied, and instead will continue to be applied any time `options.fetchPolicy` is reset to another value, until/unless the `options.nextFetchPolicy` property is removed from `options`. <br/>
+  [@benjamn](https://github.com/benjamn) in [#8465](https://github.com/apollographql/apollo-client/pull/8465)
+
 ### Improvements
 
 - `InMemoryCache` now _guarantees_ that any two result objects returned by the cache (from `readQuery`, `readFragment`, etc.) will be referentially equal (`===`) if they are deeply equal. Previously, `===` equality was often achievable for results for the same query, on a best-effort basis. Now, equivalent result objects will be automatically shared among the result trees of completely different queries. This guarantee is important for taking full advantage of optimistic updates that correctly guess the final data, and for "pure" UI components that can skip re-rendering when their input data are unchanged. <br/>

src/__tests__/client.ts

@@ -3319,7 +3319,6 @@ describe('@connection', () => {
               // again to perform an additional transition.
               this.nextFetchPolicy = fetchPolicy => {
                 ++nextFetchPolicyCallCount;
-                expect(fetchPolicy).toBe("cache-and-network");
                 return "cache-first";
               };
               return "cache-and-network";
@@ -3372,9 +3371,8 @@ describe('@connection', () => {
           client.cache.evict({ fieldName: "count" });
         } else if (handleCount === 6) {
           expect(result.data).toEqual({ count: 2 });
-          expect(nextFetchPolicyCallCount).toBe(3);
+          expect(nextFetchPolicyCallCount).toBe(4);
           expect(obs.options.fetchPolicy).toBe("cache-first");
-          expect(obs.options.nextFetchPolicy).toBeUndefined();
           setTimeout(resolve, 50);
         } else {
           reject("too many results");

src/core/ObservableQuery.ts

@@ -21,6 +21,7 @@ import {
   WatchQueryOptions,
   FetchMoreQueryOptions,
   SubscribeToMoreOptions,
+  WatchQueryFetchPolicy,
 } from './watchQueryOptions';
 import { QueryInfo } from './QueryInfo';
 
@@ -57,6 +58,10 @@ export class ObservableQuery<
     return this.options.variables;
   }
 
+  // Original value of this.options.fetchPolicy (defaulting to "cache-first"),
+  // from whenever the ObservableQuery was first created.
+  private initialFetchPolicy: WatchQueryFetchPolicy;
+
   private isTornDown: boolean;
   private queryManager: QueryManager<any>;
   private observers = new Set<Observer<ApolloQueryResult<TData>>>();
@@ -129,6 +134,8 @@ export class ObservableQuery<
     const opDef = getOperationDefinition(options.query);
     this.queryName = opDef && opDef.name && opDef.name.value;
 
+    this.initialFetchPolicy = options.fetchPolicy || "cache-first";
+
     // related classes
     this.queryManager = queryManager;
     this.queryInfo = queryInfo;
@@ -283,8 +290,6 @@ export class ObservableQuery<
       reobserveOptions.fetchPolicy = 'no-cache';
     } else if (fetchPolicy !== 'cache-and-network') {
       reobserveOptions.fetchPolicy = 'network-only';
-      // Go back to the original options.fetchPolicy after this refetch.
-      reobserveOptions.nextFetchPolicy = fetchPolicy || "cache-first";
     }
 
     if (variables && !equal(this.options.variables, variables)) {
@@ -475,23 +480,11 @@ once, rather than every time you call fetchMore.`);
       return Promise.resolve();
     }
 
-    let { fetchPolicy = 'cache-first' } = this.options;
-    const reobserveOptions: Partial<WatchQueryOptions<TVariables, TData>> = {
-      fetchPolicy,
+    return this.reobserve({
+      // Reset options.fetchPolicy to its original value.
+      fetchPolicy: this.initialFetchPolicy,
       variables,
-    };
-
-    if (fetchPolicy !== 'cache-first' &&
-        fetchPolicy !== 'no-cache' &&
-        fetchPolicy !== 'network-only') {
-      reobserveOptions.fetchPolicy = 'cache-and-network';
-      reobserveOptions.nextFetchPolicy = fetchPolicy;
-    }
-
-    return this.reobserve(
-      reobserveOptions,
-      NetworkStatus.setVariables,
-    );
+    }, NetworkStatus.setVariables);
   }
 
   public updateQuery<TVars = TVariables>(
@@ -586,7 +579,6 @@ once, rather than every time you call fetchMore.`);
         if (!isNetworkRequestInFlight(this.queryInfo.networkStatus)) {
           this.reobserve({
             fetchPolicy: "network-only",
-            nextFetchPolicy: this.options.fetchPolicy || "cache-first",
           }, NetworkStatus.poll).then(poll, poll);
         } else {
           poll();
@@ -622,26 +614,55 @@ once, rather than every time you call fetchMore.`);
     newNetworkStatus?: NetworkStatus,
   ): Promise<ApolloQueryResult<TData>> {
     this.isTornDown = false;
-    let options: WatchQueryOptions<TVariables, TData>;
-    if (newNetworkStatus === NetworkStatus.refetch) {
-      options = Object.assign({}, this.options, compact(newOptions));
-    } else {
-      if (newOptions) {
-        Object.assign(this.options, compact(newOptions));
-      }
 
+    const useDisposableConcast =
+      // Refetching uses a disposable Concast to allow refetches using different
+      // options/variables, without permanently altering the options of the
+      // original ObservableQuery.
+      newNetworkStatus === NetworkStatus.refetch ||
+      // The fetchMore method does not actually call the reobserve method, but,
+      // if it did, it would definitely use a disposable Concast.
+      newNetworkStatus === NetworkStatus.fetchMore ||
+      // Polling uses a disposable Concast so the polling options (which force
+      // fetchPolicy to be "network-only") won't override the original options.
+      newNetworkStatus === NetworkStatus.poll;
+
+    // Save the old variables, since Object.assign may modify them below.
+    const oldVariables = this.options.variables;
+
+    const options = useDisposableConcast
+      // Disposable Concast fetches receive a shallow copy of this.options
+      // (merged with newOptions), leaving this.options unmodified.
+      ? compact(this.options, newOptions)
+      : Object.assign(this.options, compact(newOptions));
+
+    if (!useDisposableConcast) {
+      // We can skip calling updatePolling if we're not changing this.options.
       this.updatePolling();
-      options = this.options;
+
+      // Reset options.fetchPolicy to its original value when variables change,
+      // unless a new fetchPolicy was provided by newOptions.
+      if (
+        newOptions &&
+        newOptions.variables &&
+        !newOptions.fetchPolicy &&
+        !equal(newOptions.variables, oldVariables)
+      ) {
+        options.fetchPolicy = this.initialFetchPolicy;
+        if (newNetworkStatus === void 0) {
+          newNetworkStatus = NetworkStatus.setVariables;
+        }
+      }
     }
 
     const concast = this.fetch(options, newNetworkStatus);
-    if (newNetworkStatus !== NetworkStatus.refetch) {
-      // We use the {add,remove}Observer methods directly to avoid
-      // wrapping observer with an unnecessary SubscriptionObserver
-      // object, in part so that we can remove it here without triggering
-      // any unsubscriptions, because we just want to ignore the old
-      // observable, not prematurely shut it down, since other consumers
-      // may be awaiting this.concast.promise.
+
+    if (!useDisposableConcast) {
+      // We use the {add,remove}Observer methods directly to avoid wrapping
+      // observer with an unnecessary SubscriptionObserver object, in part so
+      // that we can remove it here without triggering any unsubscriptions,
+      // because we just want to ignore the old observable, not prematurely shut
+      // it down, since other consumers may be awaiting this.concast.promise.
       if (this.concast) {
         this.concast.removeObserver(this.observer, true);
       }
@@ -650,6 +671,7 @@ once, rather than every time you call fetchMore.`);
     }
 
     concast.addObserver(this.observer);
+
     return concast.promise;
   }
 
@@ -732,11 +754,6 @@ export function applyNextFetchPolicy<TData, TVars>(
   } = options;
 
   if (nextFetchPolicy) {
-    // The options.nextFetchPolicy transition should happen only once, but it
-    // should also be possible (though uncommon) for a nextFetchPolicy function
-    // to set options.nextFetchPolicy to perform an additional transition.
-    options.nextFetchPolicy = void 0;
-
     // 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

src/core/__tests__/ObservableQuery.ts

@@ -978,22 +978,43 @@ describe('ObservableQuery', () => {
 
       const mocks = mockFetchQuery(queryManager);
 
-      subscribeAndCount(reject, observable, handleCount => {
-        if (handleCount === 1) {
+      subscribeAndCount(reject, observable, (count, result) => {
+        if (count === 1) {
+          expect(result).toEqual({
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+            data: dataOne,
+          });
+
           observable.refetch(differentVariables);
-        } else if (handleCount === 2) {
+
+        } else if (count === 2) {
+          expect(result).toEqual({
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+            data: dataTwo,
+          });
+
           const fqbpCalls = mocks.fetchQueryByPolicy.mock.calls;
           expect(fqbpCalls.length).toBe(2);
+          expect(fqbpCalls[0][1].fetchPolicy).toEqual('cache-first');
           expect(fqbpCalls[1][1].fetchPolicy).toEqual('network-only');
+
+          const fqoCalls = mocks.fetchQueryObservable.mock.calls;
+          expect(fqoCalls.length).toBe(2);
+          expect(fqoCalls[0][1].fetchPolicy).toEqual('cache-first');
+          expect(fqoCalls[1][1].fetchPolicy).toEqual('network-only');
+
           // 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, thanks to options.nextFetchPolicy.
           expect(observable.options.fetchPolicy).toBe('cache-first');
-          const fqoCalls = mocks.fetchQueryObservable.mock.calls;
-          expect(fqoCalls.length).toBe(2);
-          expect(fqoCalls[1][1].fetchPolicy).toEqual('cache-first');
-          resolve();
+
+          // Give the test time to fail if more results are delivered.
+          setTimeout(resolve, 50);
+        } else {
+          reject(new Error(`too many results (${count}, ${result})`));
         }
       });
     });
@@ -1117,6 +1138,118 @@ describe('ObservableQuery', () => {
       });
     });
 
+    itAsync('resets fetchPolicy when variables change when using nextFetchPolicy', (resolve, reject) => {
+      // This query and variables are copied from react-apollo
+      const queryWithVars = gql`
+        query people($first: Int) {
+          allPeople(first: $first) {
+            people {
+              name
+            }
+          }
+        }
+      `;
+
+      const data = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+      const variables1 = { first: 0 };
+
+      const data2 = { allPeople: { people: [{ name: 'Leia Skywalker' }] } };
+      const variables2 = { first: 1 };
+
+      const queryManager = mockQueryManager(
+        reject,
+        {
+          request: {
+            query: queryWithVars,
+            variables: variables1,
+          },
+          result: { data },
+        },
+        {
+          request: {
+            query: queryWithVars,
+            variables: variables2,
+          },
+          result: { data: data2 },
+        },
+        {
+          request: {
+            query: queryWithVars,
+            variables: variables1,
+          },
+          result: { data },
+        },
+        {
+          request: {
+            query: queryWithVars,
+            variables: variables2,
+          },
+          result: { data: data2 },
+        },
+      );
+
+      const observable = queryManager.watchQuery({
+        query: queryWithVars,
+        variables: variables1,
+        fetchPolicy: 'cache-and-network',
+        nextFetchPolicy: 'cache-first',
+        notifyOnNetworkStatusChange: true,
+      });
+
+      expect(observable.options.fetchPolicy).toBe('cache-and-network');
+      expect(observable["initialFetchPolicy"]).toBe('cache-and-network');
+
+      subscribeAndCount(reject, observable, (handleCount, result) => {
+        expect(result.error).toBeUndefined();
+
+        if (handleCount === 1) {
+          expect(result.data).toEqual(data);
+          expect(result.loading).toBe(false);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+          observable.refetch(variables2);
+        } else if (handleCount === 2) {
+          expect(result.loading).toBe(true);
+          expect(result.networkStatus).toBe(NetworkStatus.setVariables);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+        } else if (handleCount === 3) {
+          expect(result.data).toEqual(data2);
+          expect(result.loading).toBe(false);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+          observable.setOptions({
+            variables: variables1,
+          }).then(result => {
+            expect(result.data).toEqual(data);
+          }).catch(reject);
+          expect(observable.options.fetchPolicy).toBe('cache-and-network');
+        } else if (handleCount === 4) {
+          expect(result.loading).toBe(true);
+          expect(result.networkStatus).toBe(NetworkStatus.setVariables);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+        } else if (handleCount === 5) {
+          expect(result.data).toEqual(data);
+          expect(result.loading).toBe(false);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+          observable.reobserve({
+            variables: variables2,
+          }).then(result => {
+            expect(result.data).toEqual(data2);
+          }).catch(reject);
+          expect(observable.options.fetchPolicy).toBe('cache-and-network');
+        } else if (handleCount === 6) {
+          expect(result.data).toEqual(data2);
+          expect(result.loading).toBe(true);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+        } else if (handleCount === 7) {
+          expect(result.data).toEqual(data2);
+          expect(result.loading).toBe(false);
+          expect(observable.options.fetchPolicy).toBe('cache-first');
+          setTimeout(resolve, 10);
+        } else {
+          reject(`too many renders (${handleCount})`);
+        }
+      });
+    });
+
     itAsync('cache-and-network refetch should run @client(always: true) resolvers when network request fails', (resolve, reject) => {
       const query = gql`
         query MixedQuery {