Ensure remounted components properly adhere to fetchPolicy and nextFetchPolicy

src/core/ObservableQuery.ts

@@ -270,7 +270,6 @@ export class ObservableQuery<
         // terminates after a complete cache read, we can assume the next result
         // we receive will have NetworkStatus.ready and !loading.
         if (
-          diff.complete &&
           result.networkStatus === NetworkStatus.loading &&
           (fetchPolicy === 'cache-first' ||
           fetchPolicy === 'cache-only')

src/react/hooks/__tests__/useQuery.test.tsx

@@ -1,5 +1,5 @@
 import { RenderResult } from "@testing-library/react-hooks/src/types";
-import React, { Fragment, useEffect, useState } from 'react';
+import React, { Fragment, ReactNode, useEffect, useState } from 'react';
 import { DocumentNode, GraphQLError } from 'graphql';
 import gql from 'graphql-tag';
 import { act } from 'react-dom/test-utils';
@@ -4925,6 +4925,102 @@ describe('useQuery Hook', () => {
     });
   });
 
+  // https://github.com/apollographql/apollo-client/issues/10222
+  describe('regression test issue #10222', () => {
+    it('maintains initial fetch policy when component unmounts and remounts', async () => {
+      let helloCount = 1;
+      const query = gql`{ hello }`;
+      const link = new ApolloLink(() => {
+        return new Observable(observer => {
+          const timer = setTimeout(() => {
+            console.log('test observer.next', helloCount)
+            observer.next({ data: { hello: `hello ${helloCount++}` } });
+            observer.complete();
+          }, 50);
+
+          return () => {
+            clearTimeout(timer);
+          }
+        })
+      })
+
+      const cache = new InMemoryCache();
+
+      const client = new ApolloClient({
+        link,
+        cache
+      });
+
+      let setShow: Function
+      const Toggler = ({ children }: { children: ReactNode }) => {
+        const [show, _setShow] = useState(true);
+        setShow = _setShow;
+
+        return show ? <>{children}</> : null;
+      }
+
+      const counts = { mount: 0, unmount: 0 };
+
+      const { result, waitForNextUpdate } = renderHook(
+        () => {
+          useEffect(() => {
+            counts.mount++;
+
+            return () => {
+              counts.unmount++;
+            }
+          }, []);
+
+          const result = useQuery(query, {
+            fetchPolicy: 'network-only',
+            nextFetchPolicy: 'cache-first'
+          });
+
+          return result
+        },
+        {
+          wrapper: ({ children }) => (
+            <ApolloProvider client={client}>
+              <Toggler>{children}</Toggler>
+            </ApolloProvider>
+          ),
+        },
+      );
+
+      expect(counts).toEqual({ mount: 1, unmount: 0 });
+      expect(result.current.loading).toBe(true);
+      expect(result.current.data).toBeUndefined();
+
+      await waitForNextUpdate();
+
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({ hello: 'hello 1' });
+      expect(cache.readQuery({ query })).toEqual({ hello: 'hello 1' })
+
+      act(() => {
+        setShow(false);
+      });
+
+      expect(counts).toEqual({ mount: 1, unmount: 1 });
+
+      act(() => {
+        setShow(true);
+      });
+
+      expect(counts).toEqual({ mount: 2, unmount: 1 });
+
+      expect(result.current.loading).toBe(true);
+      expect(result.current.data).toBeUndefined();
+
+      await waitForNextUpdate();
+
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({ hello: 'hello 2' });
+      expect(cache.readQuery({ query })).toEqual({ hello: 'hello 2' })
+    });
+  });
+
+
   describe('defer', () => {
     it('should handle deferred queries', async () => {
       const query = gql`
@@ -5542,6 +5638,7 @@ describe('useQuery Hook', () => {
 
       expect(result.current.loading).toBe(true);
       expect(result.current.data).toBe(undefined);
+      expect(result.current.networkStatus).toBe(NetworkStatus.loading);
       setTimeout(() => {
         link.simulateResult({
           result: {
@@ -5621,6 +5718,7 @@ describe('useQuery Hook', () => {
       expect(result.current.label).toBe(undefined);
       // @ts-ignore
       expect(result.current.extensions).toBe(undefined);
+      expect(result.current.networkStatus).toBe(NetworkStatus.error);
       expect(result.current.error).toBeInstanceOf(ApolloError);
       expect(result.current.error!.message).toBe('homeWorld for character with ID 1000 could not be fetched.');
 

src/react/hooks/useQuery.ts

@@ -27,6 +27,8 @@ import {
   QueryResult,
   ObservableQueryFields,
 } from '../types/types';
+import { isNetworkRequestInFlight } from '../../core/networkStatus';
+import { logMissingFieldErrors } from '../../core/ObservableQuery';
 
 import { DocumentType, verifyDocumentType } from '../parser';
 import { useApolloClient } from './useApolloClient';
@@ -140,13 +142,77 @@ class InternalState<TData, TVariables> {
           return () => {};
         }
 
-        const onNext = () => {
+        const onNext = (result: ApolloQueryResult<TData>) => {
           const previousResult = this.result;
-          // We use `getCurrentResult()` instead of the onNext argument because
-          // the values differ slightly. Specifically, loading results will have
-          // an empty object for data instead of `undefined` for some reason.
-          const result = obsQuery.getCurrentResult();
-          // Make sure we're not attempting to re-render similar results
+          const diff = obsQuery['queryInfo'].getDiff();
+
+          // Unfortunately the result passed into `onNext` doesn't seem to have
+          // canonization applied, whereas the `diff.result` does. Theoretically
+          // it should as result.data when read from the cache also uses the
+          // diff (https://github.com/apollographql/apollo-client/blob/6875d3ced43162557cbd507558dfbdbc37512a69/src/core/QueryManager.ts#L1392)
+          // but I can't seem to figure out why it isn't applied. This patches
+          // the behavior to ensure we retain it.
+          if (obsQuery.options.canonizeResults) {
+            result.data = diff.result;
+          }
+
+          // Previously, this code called `obsQuery.getCurrentResult()` to get
+          // the result returned in `useQuery` rather than relying on the
+          // argument passed to `onNext`. Unfortunately the `result` passed as
+          // the argument doesn't always resemble the result returned from
+          // `obsQuery.getCurrentResult()`. Because of this, we have to patch
+          // some of that behavior here to ensure the result maintains the same
+          // information.
+          //
+          // Why can't we use `obsQuery.getCurrentResult()`? #9823 fixed an
+          // issue with `skip` and the fetch policy. In that fix, the
+          // `nextFetchPolicy` was changed to be synchronously set as soon as we
+          // kick off the query. Unfortunately this has the side effect that
+          // `obsQuery.getCurrentResult()` uses the new fetch policy too early.
+          // In some cases, this meant we'd return cached data when we didn't
+          // mean to, such as when a component is unmounted, then mounted again
+          // (see #10222).
+          //
+          // We should really look to refactor this code out of here for AC v4.
+          // This behavior should really be patched in QueryManager, but I was
+          // afraid that developers might rely on the existing behavior
+          // (such as returning empty objects instead of setting those as
+          // undefined). As such, I decided to patch it only in useQuery.
+          if (!diff.complete) {
+            if (!hasOwnProperty.call(result, 'partial')) {
+              result.partial = true;
+            }
+
+            if (
+              // Deferred queries by nature return partial results, so we want
+              // to ensure we return that data, even if `returnPartialData` is
+              // set to false. Therefore we only reset `data` to `undefined`
+              // when the request is in flight.
+              isNetworkRequestInFlight(result.networkStatus) &&
+                hasOwnProperty.call(result, 'data') &&
+                !this.getObsQueryOptions().returnPartialData
+            ) {
+              result.data = void 0 as any;
+            }
+
+            // Retain logging used in `obsQuery.getCurrentResult()` when this
+            // was switched over to read the result from the argument.
+            if (
+              __DEV__ &&
+              !obsQuery.options.partialRefetch &&
+              !result.loading &&
+              !result.data &&
+              !result.error
+            ) {
+              logMissingFieldErrors(diff.missing);
+            }
+          }
+
+          if (equal(result.data, {})) {
+            result.data = void 0 as any;
+          }
+
+          // make sure we're not attempting to re-render similar results
           if (
             previousResult &&
             previousResult.loading === result.loading &&