Preserve no-cache fetchPolicy with notifyOnNetworkStatusChange

CHANGELOG.md

@@ -5,6 +5,8 @@
 - Maintain serial ordering of `asyncMap` mapping function calls, and prevent potential unhandled `Promise` rejection errors. <br/>
   [@benjamn](https://github.com/benjamn) in [#7818](https://github.com/apollographql/apollo-client/pull/7818)
 
+- Preserve fetch policy even when `notifyOnNetworkStatusChange` is set <br />
+  [@jcreighton](https://github.com/jcreighton) in [#7761](https://github.com/apollographql/apollo-client/pull/7761)
 ## Apollo Client 3.3.11
 
 ### Bug fixes

src/core/QueryManager.ts

@@ -914,7 +914,6 @@ export class QueryManager<TStore> {
     const query = this.transform(options.query).document;
     const variables = this.getVariables(query, options.variables) as TVars;
     const queryInfo = this.getQuery(queryId);
-    const oldNetworkStatus = queryInfo.networkStatus;
 
     let {
       fetchPolicy = "cache-first" as WatchQueryFetchPolicy,
@@ -924,26 +923,6 @@ export class QueryManager<TStore> {
       context = {},
     } = options;
 
-    const mightUseNetwork =
-      fetchPolicy === "cache-first" ||
-      fetchPolicy === "cache-and-network" ||
-      fetchPolicy === "network-only" ||
-      fetchPolicy === "no-cache";
-
-    if (mightUseNetwork &&
-        notifyOnNetworkStatusChange &&
-        typeof oldNetworkStatus === "number" &&
-        oldNetworkStatus !== networkStatus &&
-        isNetworkRequestInFlight(networkStatus)) {
-      // In order to force delivery of an incomplete cache result with
-      // loading:true, we tweak the fetchPolicy to include the cache, and
-      // pretend that returnPartialData was enabled.
-      if (fetchPolicy !== "cache-first") {
-        fetchPolicy = "cache-and-network";
-      }
-      returnPartialData = true;
-    }
-
     const normalized = Object.assign({}, options, {
       query,
       variables,
@@ -1038,8 +1017,11 @@ export class QueryManager<TStore> {
       errorPolicy,
       returnPartialData,
       context,
+      notifyOnNetworkStatusChange,
     } = options;
 
+    const oldNetworkStatus = queryInfo.networkStatus;
+
     queryInfo.init({
       document: query,
       variables,
@@ -1092,6 +1074,13 @@ export class QueryManager<TStore> {
         errorPolicy,
       });
 
+    const shouldNotifyOnNetworkStatusChange = () => (
+      notifyOnNetworkStatusChange &&
+      typeof oldNetworkStatus === "number" &&
+      oldNetworkStatus !== networkStatus &&
+      isNetworkRequestInFlight(networkStatus)
+    );
+
     switch (fetchPolicy) {
     default: case "cache-first": {
       const diff = readCache();
@@ -1109,6 +1098,13 @@ export class QueryManager<TStore> {
         ];
       }
 
+      if (shouldNotifyOnNetworkStatusChange()) {
+        return [
+          resultsFromCache(diff),
+          resultsFromLink(true),
+        ];
+      }
+
       return [
         resultsFromLink(true),
       ];
@@ -1117,7 +1113,7 @@ export class QueryManager<TStore> {
     case "cache-and-network": {
       const diff = readCache();
 
-      if (diff.complete || returnPartialData) {
+      if (diff.complete || returnPartialData || shouldNotifyOnNetworkStatusChange()) {
         return [
           resultsFromCache(diff),
           resultsFromLink(true),
@@ -1135,9 +1131,22 @@ export class QueryManager<TStore> {
       ];
 
     case "network-only":
+      if (shouldNotifyOnNetworkStatusChange()) {
+        const diff = readCache();
+
+        return [
+          resultsFromCache(diff),
+          resultsFromLink(true),
+        ];
+      }
+
       return [resultsFromLink(true)];
 
     case "no-cache":
+      if (shouldNotifyOnNetworkStatusChange()) {
+          return [resultsFromCache(queryInfo.getDiff()), resultsFromLink(false)];
+      }
+
       return [resultsFromLink(false)];
 
     case "standby":

src/core/__tests__/fetchPolicies.ts

@@ -324,6 +324,163 @@ describe('no-cache', () => {
       })
       .then(resolve, reject);
   });
+
+  describe('when notifyOnNetworkStatusChange is set', () => {
+    itAsync('does not save the data to the cache on success', (resolve, reject) => {
+      let called = 0;
+      const inspector = new ApolloLink((operation, forward) => {
+        called++;
+        return forward(operation).map(result => {
+          called++;
+          return result;
+        });
+      });
+
+      const client = new ApolloClient({
+        link: inspector.concat(createLink(reject)),
+        cache: new InMemoryCache({ addTypename: false }),
+      });
+
+      return client.query({ query, fetchPolicy: 'no-cache', notifyOnNetworkStatusChange: true }).then(
+        () => client.query({ query }).then(actualResult => {
+          expect(stripSymbols(actualResult.data)).toEqual(result);
+          // the second query couldn't read anything from the cache
+          expect(called).toBe(4);
+        }),
+      ).then(resolve, reject);
+    });
+
+    itAsync('does not save data to the cache on failure', (resolve, reject) => {
+      let called = 0;
+      const inspector = new ApolloLink((operation, forward) => {
+        called++;
+        return forward(operation).map(result => {
+          called++;
+          return result;
+        });
+      });
+
+      const client = new ApolloClient({
+        link: inspector.concat(createFailureLink()),
+        cache: new InMemoryCache({ addTypename: false }),
+      });
+
+      let didFail = false;
+      return client
+        .query({ query, fetchPolicy: 'no-cache', notifyOnNetworkStatusChange: true })
+        .catch(e => {
+          expect(e.message).toMatch('query failed');
+          didFail = true;
+        })
+        .then(() => client.query({ query }).then(actualResult => {
+          expect(stripSymbols(actualResult.data)).toEqual(result);
+          // the first error doesn't call .map on the inspector
+          expect(called).toBe(3);
+          expect(didFail).toBe(true);
+        }))
+        .then(resolve, reject);
+    });
+
+    itAsync('gives appropriate networkStatus for watched queries', (resolve, reject) => {
+      const client = new ApolloClient({
+        link: ApolloLink.empty(),
+        cache: new InMemoryCache(),
+        resolvers: {
+          Query: {
+            hero(_data, args) {
+              return {
+                __typename: 'Hero',
+                ...args,
+                name: 'Luke Skywalker',
+              };
+            },
+          },
+        },
+      });
+
+      const observable = client.watchQuery({
+        query: gql`
+          query FetchLuke($id: String) {
+            hero(id: $id) @client {
+              id
+              name
+            }
+          }
+        `,
+        fetchPolicy: 'no-cache',
+        variables: { id: '1' },
+        notifyOnNetworkStatusChange: true,
+      });
+
+      function dataWithId(id: number | string) {
+        return {
+          hero: {
+            __typename: 'Hero',
+            id: String(id),
+            name: 'Luke Skywalker',
+          },
+        };
+      }
+
+      subscribeAndCount(reject, observable, (count, result) => {
+        if (count === 1) {
+          expect(result).toEqual({
+            data: dataWithId(1),
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+          return observable.setVariables({ id: '2' });
+        } else if (count === 2) {
+          expect(result).toEqual({
+            data: {},
+            loading: true,
+            networkStatus: NetworkStatus.setVariables,
+            partial: true,
+          });
+        } else if (count === 3) {
+          expect(result).toEqual({
+            data: dataWithId(2),
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+          return observable.refetch();
+        } else if (count === 4) {
+          expect(result).toEqual({
+            data: dataWithId(2),
+            loading: true,
+            networkStatus: NetworkStatus.refetch,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+        } else if (count === 5) {
+          expect(result).toEqual({
+            data: dataWithId(2),
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+          return observable.refetch({ id: '3' });
+        } else if (count === 6) {
+          expect(result).toEqual({
+            data: {},
+            loading: true,
+            networkStatus: NetworkStatus.setVariables,
+            partial: true,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+        } else if (count === 7) {
+          expect(result).toEqual({
+            data: dataWithId(3),
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+          });
+          expect(client.cache.extract(true)).toEqual({});
+          resolve();
+        }
+      });
+    });
+  });
 });
 
 describe('cache-first', () => {