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

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();
@@ -323,19 +325,7 @@ export class QueryData<TData, TVariables> extends OperationData {
     let result: any = this.observableQueryFields();
     const options = this.getOptions();
 
-    // 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).
-    if (options.skip) {
-      result = {
-        ...result,
-        data: undefined,
-        error: undefined,
-        loading: false,
-        called: true
-      };
-    } else if (this.currentObservable) {
-      // Fetch the current result (if any) from the store.
+    if (this.currentObservable) {
       const currentResult = this.currentObservable.getCurrentResult();
       const { data, loading, partial, networkStatus, errors } = currentResult;
       let { error } = currentResult;
@@ -355,7 +345,7 @@ export class QueryData<TData, TVariables> extends OperationData {
         called: true
       };
 
-      if (loading) {
+      if (loading || options.skip) {
         // Fall through without modifying result...
       } else if (error) {
         Object.assign(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({
@@ -620,32 +625,53 @@ describe('[queries] skip', () => {
     })(
       class extends React.Component<any> {
         render() {
+          console.log(this.props);
           switch (++count) {
             case 1:
               expect(this.props.data.loading).toBe(true);
               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 +699,7 @@ describe('[queries] skip', () => {
     );
 
     await wait(() => {
-      expect(count).toEqual(5);
+      expect(count).toEqual(6);
     });
   });
 

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

@@ -526,7 +526,7 @@ describe('useQuery Hook', () => {
             break;
           case 3:
             expect(loading).toBeFalsy();
-            expect(data).toBeUndefined();
+            expect(data).toEqual(CAR_RESULT_DATA);
             break;
           case 4:
             throw new Error('Uh oh - we should have stopped polling!');
@@ -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).toEqual(CAR_RESULT_DATA);
+            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);
+    });
   });
 });