Move cache-first fallback logic after first network request.

PR #6353 explains the rationale for switching to a cache-first FetchPolicy
after an initial cache-and-network or network-only policy.

When #6365 was implemented, options.fetchPolicy was examined only once, at
the beginning of fetchQueryObservable, so the timing of changing
options.fetchPolicy did not matter as much. However, fixing #6659 involves
checking the current options.fetchPolicy whenever the QueryData class
calls this.currentObservable.getCurrentResult(), so it's now more
important to delay changing options.fetchPolicy until after the first
network request has completed.

src/core/QueryManager.ts

@@ -836,24 +836,6 @@ 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" ||
@@ -924,7 +906,27 @@ export class QueryManager<TStore> {
         : fromVariables(normalized.variables!)
     );
 
-    concast.cleanup(() => this.fetchCancelFns.delete(queryId));
+    concast.cleanup(() => {
+      this.fetchCancelFns.delete(queryId);
+
+      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";
+      }
+    });
 
     return concast;
   }

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

@@ -619,32 +619,35 @@ describe('[queries] skip', () => {
     })(
       class extends React.Component<any> {
         render() {
-          switch (count) {
-            case 0:
-              expect(this.props.data.loading).toBeTruthy();
+          switch (++count) {
+            case 1:
+              expect(this.props.data.loading).toBe(true);
               expect(ranQuery).toBe(1);
               break;
-            case 1:
-              expect(this.props.data.loading).toBeFalsy();
+            case 2:
+              expect(this.props.data.loading).toBe(false);
               expect(ranQuery).toBe(1);
               setTimeout(() => {
                 this.props.setSkip(true);
               });
               break;
-            case 2:
+            case 3:
               expect(this.props.data).toBeUndefined();
               expect(ranQuery).toBe(1);
               setTimeout(() => {
                 this.props.setSkip(false);
               });
               break;
-            case 3:
-              expect(this.props.data!.loading).toBeFalsy();
-              expect(ranQuery).toBe(2);
+            case 4:
+              expect(this.props.data!.loading).toBe(true);
+              expect(ranQuery).toBe(3);
+              break;
+            case 5:
+              expect(this.props.data!.loading).toBe(false);
+              expect(ranQuery).toBe(3);
               break;
             default:
           }
-          count += 1;
           return null;
         }
       }
@@ -669,7 +672,7 @@ describe('[queries] skip', () => {
     );
 
     await wait(() => {
-      expect(count).toEqual(4);
+      expect(count).toEqual(5);
     });
   });