Avoid making network requests when skip is true

When skip is set to true but other updated options have been
passed into `useQuery` (like updated variables), `useQuery` will
still make a network request, even though it will skip the
handling of the response. This commit makes sure `useQuery`
doesn't make unnecessary `skip` network requests.

Fixes #6670
Fixes #6190
Fixes #6572

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Apollo Client 3.1.2 (not yet released)
+
+## Bug Fixes
+
+- Avoid making network requests when `skip` is `true`.  <br/>
+  [@hwillson](https://github.com/hwillson) in [#6752](https://github.com/apollographql/apollo-client/pull/6752)
+
 ## Apollo Client 3.1.1
 
 ## Bug Fixes

src/react/data/QueryData.ts

@@ -227,6 +227,8 @@ export class QueryData<TData, TVariables> extends OperationData {
   }
 
   private updateObservableQuery() {
+    if (this.getOptions().skip) return;
+
     // If we skipped initially, we may not have yet created the observable
     if (!this.currentObservable) {
       this.initializeObservableQuery();
@@ -326,6 +328,13 @@ export class QueryData<TData, TVariables> extends OperationData {
     // When skipping a query (ie. we're not querying for data but still want
     // to render children), make sure the `data` is cleared out and
     // `loading` is set to `false` (since we aren't loading anything).
+    //
+    // NOTE: We no longer think this is the correct behavior. Skipping should
+    // not automatically set `data` to `undefined`, but instead leave the
+    // previous data in place. In other words, skipping should not mandate
+    // that previously received data is all of a sudden removed. Unfortunately,
+    // changing this is breaking, so we'll have to wait until Apollo Client
+    // 4.0 to address this.
     if (options.skip) {
       result = {
         ...result,

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

@@ -596,11 +596,16 @@ describe('[queries] skip', () => {
       ranQuery++;
       return f ? f(o) : null;
     }).concat(
-      mockSingleLink({
-        request: { query },
-        result: { data },
-        newData: () => ({ data: nextData })
-      })
+      mockSingleLink(
+        {
+          request: { query },
+          result: { data },
+        },
+        {
+          request: { query },
+          result: { data: nextData },
+        },
+      )
     );
 
     const client = new ApolloClient({
@@ -626,26 +631,46 @@ describe('[queries] skip', () => {
               expect(ranQuery).toBe(1);
               break;
             case 2:
+              // The first batch of data is fetched over the network, and
+              // verified here, followed by telling the component we want to
+              // skip running subsequent queries.
               expect(this.props.data.loading).toBe(false);
+              expect(this.props.data.allPeople).toEqual(data.allPeople);
               expect(ranQuery).toBe(1);
               setTimeout(() => {
                 this.props.setSkip(true);
               });
               break;
             case 3:
+              // This render is triggered after setting skip to true. Now
+              // let's set skip to false to re-trigger the query.
               expect(this.props.data).toBeUndefined();
               expect(ranQuery).toBe(1);
               setTimeout(() => {
                 this.props.setSkip(false);
               });
               break;
             case 4:
-              expect(this.props.data!.loading).toBe(true);
-              expect(ranQuery).toBe(3);
+              // Since the `nextFetchPolicy` was set to `cache-first`, our
+              // query isn't loading as it's able to find the result of the
+              // query directly from the cache. Let's trigger a refetch
+              // to manually load the next batch of data.
+              expect(this.props.data!.loading).toBe(false);
+              expect(this.props.data.allPeople).toEqual(data.allPeople);
+              expect(ranQuery).toBe(1);
+              setTimeout(() => {
+                this.props.data.refetch();
+              });
               break;
             case 5:
+              expect(this.props.data!.loading).toBe(true);
+              expect(ranQuery).toBe(2);
+              break;
+            case 6:
+              // The next batch of data has loaded.
               expect(this.props.data!.loading).toBe(false);
-              expect(ranQuery).toBe(3);
+              expect(this.props.data.allPeople).toEqual(nextData.allPeople);
+              expect(ranQuery).toBe(2);
               break;
             default:
           }
@@ -673,7 +698,7 @@ describe('[queries] skip', () => {
     );
 
     await wait(() => {
-      expect(count).toEqual(5);
+      expect(count).toEqual(6);
     });
   });
 

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

@@ -2162,5 +2162,68 @@ describe('useQuery Hook', () => {
         expect(renderCount).toBe(3);
       }).then(resolve, reject);
     });
+
+    itAsync('should not make network requests when `skip` is `true`', (resolve, reject) => {
+      let networkRequestCount = 0;
+      const link = new ApolloLink((o, f) => {
+        networkRequestCount += 1;
+        return f ? f(o) : null;
+      }).concat(mockSingleLink(
+        {
+          request: {
+            query: CAR_QUERY,
+            variables: { someVar: true }
+          },
+          result: { data: CAR_RESULT_DATA }
+        }
+      ));
+
+      const client = new ApolloClient({
+        link,
+        cache: new InMemoryCache()
+      });
+
+      let renderCount = 0;
+      const Component = () => {
+        const [skip, setSkip] = useState(false);
+        const { loading, data } = useQuery(CAR_QUERY, {
+          fetchPolicy: 'no-cache',
+          skip,
+          variables: { someVar: !skip }
+        });
+
+        switch (++renderCount) {
+          case 1:
+            expect(loading).toBeTruthy();
+            expect(data).toBeUndefined();
+            break;
+          case 2:
+            expect(loading).toBeFalsy();
+            expect(data).toEqual(CAR_RESULT_DATA);
+            expect(networkRequestCount).toBe(1);
+            setTimeout(() => setSkip(true));
+            break;
+          case 3:
+            expect(loading).toBeFalsy();
+            expect(data).toBeUndefined();
+            expect(networkRequestCount).toBe(1);
+            break;
+          default:
+            reject('too many renders');
+        }
+
+        return null;
+      };
+
+      render(
+        <ApolloProvider client={client}>
+          <Component />
+        </ApolloProvider>
+      );
+
+      return wait(() => {
+        expect(renderCount).toBe(3);
+      }).then(resolve, reject);
+    });
   });
 });