Fall back to cache-first after cache-and-network or network-only. (#6353

)

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, which we can achieve by modifying options.fetchPolicy in
QueryManager#fetchQueryObservable 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.

Should fix #6305, and a few other related issues (TBD).

src/core/QueryManager.ts

@@ -836,6 +836,24 @@ export class QueryManager<TStore> {
       context = {},
     } = options;
 
+    if (fetchPolicy === "cache-and-network" ||
+        fetchPolicy === "network-only") {
+      // 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";
+    }
+
     const mightUseNetwork =
       fetchPolicy === "cache-first" ||
       fetchPolicy === "cache-and-network" ||

src/core/__tests__/ObservableQuery.ts

@@ -933,15 +933,29 @@ describe('ObservableQuery', () => {
   });
 
   describe('refetch', () => {
-    type TFQO = QueryManager<any>["fetchQueryObservable"];
     function mockFetchQuery(queryManager: QueryManager<any>) {
-      const origFetchQuery: TFQO = (queryManager as any).fetchQueryObservable;
-      return (queryManager as any).fetchQueryObservable = jest.fn<
-        ReturnType<TFQO>,
-        Parameters<TFQO>
+      const fetchQueryObservable = queryManager.fetchQueryObservable;
+      const fetchQueryByPolicy: QueryManager<any>["fetchQueryByPolicy"] =
+        (queryManager as any).fetchQueryByPolicy;
+
+      const mock = <T extends
+        | typeof fetchQueryObservable
+        | typeof fetchQueryByPolicy
+      >(original: T) => jest.fn<
+        ReturnType<T>,
+        Parameters<T>
       >(function () {
-        return origFetchQuery.apply(queryManager, arguments);
+        return original.apply(queryManager, arguments);
       });
+
+      const mocks = {
+        fetchQueryObservable: mock(fetchQueryObservable),
+        fetchQueryByPolicy: mock(fetchQueryByPolicy),
+      };
+
+      Object.assign(queryManager, mocks);
+
+      return mocks;
     }
 
     itAsync('calls fetchRequest with fetchPolicy `network-only` when using a non-networked fetch policy', (resolve, reject) => {
@@ -964,15 +978,24 @@ describe('ObservableQuery', () => {
         fetchPolicy: 'cache-first',
       });
 
-      const mocked = mockFetchQuery(queryManager);
+      const mocks = mockFetchQuery(queryManager);
 
       subscribeAndCount(reject, observable, handleCount => {
         if (handleCount === 1) {
           observable.refetch(differentVariables);
         } else if (handleCount === 2) {
-          expect(mocked.mock.calls[1][1].fetchPolicy).toEqual(
-            'network-only',
-          );
+          const fqbpCalls = mocks.fetchQueryByPolicy.mock.calls;
+          expect(fqbpCalls.length).toBe(2);
+          expect(fqbpCalls[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, since network-only (and cache-and-network) fetch
+          // policies fall back to cache-first after the first request.
+          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();
         }
       });
@@ -1000,15 +1023,24 @@ describe('ObservableQuery', () => {
           fetchPolicy: 'no-cache',
         });
 
-        const mocked = mockFetchQuery(queryManager);
+        const mocks = mockFetchQuery(queryManager);
 
         subscribeAndCount(reject, observable, handleCount => {
           if (handleCount === 1) {
             observable.refetch(differentVariables);
           } else if (handleCount === 2) {
-            expect(
-              mocked.mock.calls[1][1].fetchPolicy,
-            ).toEqual('no-cache');
+            const fqbpCalls = mocks.fetchQueryByPolicy.mock.calls;
+            expect(fqbpCalls.length).toBe(2);
+            expect(fqbpCalls[1][1].fetchPolicy).toBe('no-cache');
+
+            // Unlike network-only or cache-and-network, the no-cache
+            // FetchPolicy does not switch to cache-first after the first
+            // network request.
+            expect(observable.options.fetchPolicy).toBe('no-cache');
+            const fqoCalls = mocks.fetchQueryObservable.mock.calls;
+            expect(fqoCalls.length).toBe(2);
+            expect(fqoCalls[1][1].fetchPolicy).toBe('no-cache');
+
             resolve();
           }
         });
@@ -1159,34 +1191,35 @@ describe('ObservableQuery', () => {
               networkStatus: NetworkStatus.ready,
             });
 
+            const oldLinkObs = linkObservable;
             // Make the next network request fail.
             linkObservable = errorObservable;
 
             observable.refetch().then(
-              result => {
-                expect(result).toEqual({
-                  data: {
-                    counter: 3,
-                    name: 'Ben',
-                  },
-                });
+              () => {
+                reject(new Error('should have gotten an error'));
               },
+
               error => {
                 expect(error).toBe(intentionalNetworkFailure);
+
+                // Switch back from errorObservable.
+                linkObservable = oldLinkObs;
+
+                observable.refetch().then(result => {
+                  expect(result).toEqual({
+                    data: {
+                      counter: 3,
+                      name: 'Ben',
+                    },
+                    loading: false,
+                    networkStatus: NetworkStatus.ready,
+                  });
+                  resolve();
+                }, reject);
               },
             );
-          } else if (handleCount === 3) {
-            expect(result).toEqual({
-              data: {
-                counter: 3,
-                name: 'Ben',
-              },
-              loading: true,
-              networkStatus: NetworkStatus.refetch,
-            });
-
-            resolve();
-          } else if (handleCount > 4) {
+          } else if (handleCount > 2) {
             reject(new Error('should not get here'));
           }
         },
@@ -1545,38 +1578,38 @@ describe('ObservableQuery', () => {
         },
       );
 
-      queryManager.query({ query, variables }).then(() => {
+      queryManager.query({ query, variables }).then(result => {
+        expect(result).toEqual({
+          data: dataOne,
+          loading: false,
+          networkStatus: NetworkStatus.ready,
+        });
+
         const observable = queryManager.watchQuery({
           query,
           variables,
           fetchPolicy: 'network-only',
         });
-        expect(stripSymbols(observable.getCurrentResult())).toEqual({
-          data: undefined,
+
+        expect(observable.getCurrentResult()).toEqual({
+          data: void 0,
           loading: true,
           networkStatus: 1,
           partial: false,
         });
 
         subscribeAndCount(reject, observable, (handleCount, subResult) => {
-          const {
-            data,
-            loading,
-            networkStatus,
-          } = observable.getCurrentResult();
-
           if (handleCount === 1) {
             expect(subResult).toEqual({
-              data,
-              loading,
-              networkStatus,
+              loading: true,
+              networkStatus: NetworkStatus.loading,
               partial: false,
             });
           } else if (handleCount === 2) {
-            expect(stripSymbols(subResult)).toEqual({
+            expect(subResult).toEqual({
               data: dataTwo,
               loading: false,
-              networkStatus: 7,
+              networkStatus: NetworkStatus.ready,
             });
             resolve();
           }