Repository: apollographql/apollo-client

Problem statement:
[3.0-beta] Changing skip to true on a hook ignores results from "fast" data sources
In a situation where:
- we use a hook with `skip: true`
- we switch the `skip` to false
- the link responds without a delay (it may be a Promise, but it cannot use `setTimeout`) - like a caching mechanism, mock, etc.

**Intended outcome:**
The query is executed and results are displayed

**Actual outcome:**
The query is executed, but results are *not* displayed - they're "loading" forever
Switching the `skip` again to `true` and `false` again works as intended. 

**How to reproduce the issue:**
See this sandbox for a reproduction:
https://codesandbox.io/s/goofy-spence-0l96j

Click the `toggle skip` - you should see the results, but you'll see the "Loading..." screen. If you change the version of the package to `beta.14` - it will work again.

**Versions**
3.0.0-beta.29
The 3.0.0-beta.14 works as expected, so regression was introduced in beta.15.

**Potential Solution**
I was able to fix this by checking `this.options.skip` in this line instead:
https://github.com/apollographql/apollo-client/blob/master/src/react/data/QueryData.ts#L296

Currently, when the result of the query arrives, the `previousOptions` still holds `skip: true` - therefore the result is ignored. `previousOptions` are set by the hook with `afterExecute`, but it seems that this is too late for this skipping logic to work as intended.




Hints:

Patch:
diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts
--- a/src/react/data/QueryData.ts
+++ b/src/react/data/QueryData.ts
@@ -279,18 +279,6 @@ export class QueryData<TData, TVariables> extends OperationData {
           return;
         }
 
-        // If we skipped previously, `previousResult.data` is set to undefined.
-        // When this subscription is run after skipping, Apollo Client sends
-        // the last query result data alongside the `loading` true state. This
-        // means the previous skipped `data` of undefined and the incoming
-        // data won't match, which would normally mean we want to trigger a
-        // render to show the new data. In this case however we're already
-        // showing the loading state, and want to avoid triggering an
-        // additional and unnecessary render showing the same loading state.
-        if (this.previousOptions.skip) {
-          return;
-        }
-
         onNewData();
       },
       error: error => {


Test patch:
diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx
--- a/src/react/hooks/__tests__/useQuery.test.tsx
+++ b/src/react/hooks/__tests__/useQuery.test.tsx
@@ -2069,4 +2069,45 @@ describe('useQuery Hook', () => {
       }).then(resolve, reject);
     });
   });
+
+  describe('Skipping', () => {
+    itAsync('should skip running a query when `skip` is `true`', (resolve, reject) => {
+      let renderCount = 0;
+
+      const Component = () => {
+        const [skip, setSkip] = useState(true);
+        const { loading, data } = useQuery(CAR_QUERY, { skip });
+
+        switch (++renderCount) {
+          case 1:
+            expect(loading).toBeFalsy();
+            expect(data).toBeUndefined();
+            setTimeout(() => setSkip(false));
+            break;
+          case 2:
+            expect(loading).toBeTruthy();
+            expect(data).toBeUndefined();
+            break;
+          case 3:
+            expect(loading).toBeFalsy();
+            expect(data).toEqual(CAR_RESULT_DATA);
+            break;
+          default:
+            reject("too many renders");
+        }
+
+        return null;
+      };
+
+      render(
+        <MockedProvider mocks={CAR_MOCKS}>
+          <Component />
+        </MockedProvider>
+      );
+
+      return wait(() => {
+        expect(renderCount).toBe(3);
+      }).then(resolve, reject);
+    });
+  });
 });