Repository: apollographql/apollo-client

Problem statement:
Cache-and-network should not make a network call when watched cached items change via subscription/mutation
*This is new behavior as of beta .46 and has prevented us from moving past .45 (most likely a result of https://github.com/apollographql/apollo-client/pull/6221).*

**Intended outcome:**
A query with `fetchPolicy: cache-and-network` should not make a network call when a cached item that it's watching changes.  It should just re-render containing the data from the cache.

**Actual outcome:**
When an individual cached item changes (via subscription or mutation), if a query with `fetchPolicy: cache-and-network` contains that cached item, the query is making a network call to refetch.

**How to reproduce the issue:**
As instructed, I've created a full reproduction here: https://github.com/jebonfig/react-apollo-error-template

**Notes**
`cache-and-network` is an important fetchPolicy in our App, as it gives us the benefit of quick renders from cache on subsequent component mounts, and also immediately follows up with the server for consistency.  

A common example use case is:
- a `cache-and-network` list of cars query
- a `cache-only` query which pulls an individual car from the cars query data via typePolicy
  - *quick side note here is I've also noticed new warnings: "Missing cache result fields: person". If I've specified a `cache-only` fetch policy on the query, and a type policy to pull it from the list, then my item not being in the cache is a perfectly valid scenario. I'd argue a warning here is unnecessary noise*
- a `useSubscription` watching the list of cars

When we receive a subscription update for an individual car, the car in the cache is updated with the new data as expected.  However, the new, undesired behavior is that the list query now unnecessarily refetches from the network instead of just re-rendering based on the cache updates that have already been made.  

So you might say, this is intended based on the .46 changes mentioned above.  If that's the case, then how can I retain the initial onComponentMount query behavior of `cache-and-network`, _AND_ the updating behavior when responding to mutations and subscriptions of `cache-first`? 
 **In other words, I want my query to be `cache-and-network` on initial component mount, but `cache-first` when the cached data my query is watching has been changed by a mutation or subscription.**


**Versions**
```
  System:
    OS: macOS High Sierra 10.13.6
  Binaries:
    Node: 11.2.0 - /usr/local/bin/node
    npm: 6.4.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 81.0.4044.138
    Safari: 13.1
  npmPackages:
    @apollo/client: 3.0.0-beta.48 => 3.0.0-beta.48 
    @apollo/link-error: ^2.0.0-beta.3 => 2.0.0-beta.3 
    @apollo/link-ws: ^2.0.0-beta.3 => 2.0.0-beta.3 
```



Hints:
The problem also occurs with the `"network-only"` fetch policy. Whenever the cache is updated (either manually or by a mutation), the query is refetched.

Patch:
diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts
--- a/src/core/QueryManager.ts
+++ b/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" ||


Test patch:
diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts
--- a/src/core/__tests__/ObservableQuery.ts
+++ b/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();
           }