Repository: apollographql/apollo-client

Problem statement:
Function in onCompleted fires when skip is true
Hi all, I'm experiencing the same issue as reported here by @xiaoyu-tamu:
https://github.com/apollographql/react-apollo/issues/3814

I believe it was reported in the wrong place (under the old react-apollo repo), so I am reposting here for visibility.

**Intended outcome:**
Function specified in `onCompleted` should not fire when the `skip` option is _**true**_.



**Actual outcome:**
Function specified in onCompleted fires even when `skip` option is _**true**_


**How to reproduce the issue:**
@xiaoyu-tamu's minimal codesandbox
https://codesandbox.io/s/jolly-mclean-rtrwj

```
const { loading, data } = useQuery(ALL_PEOPLE, {
    skip: true,
    onCompleted: () => {
      alert("skipped but onComplete fired");
    }
  });
```

**Versions**
3.0.0-beta.24 (codesandbox minimal reproduction)
3.0.0-beta.43 (reproduced in my own project)

  System:
    OS: macOS 10.15.3
  Binaries:
    Node: 10.16.0 - ~/.nvm/versions/node/v10.16.0/bin/node
    Yarn: 1.22.4 - ~/Desktop/{redacted}/node_modules/.bin/yarn
    npm: 6.9.0 - ~/.nvm/versions/node/v10.16.0/bin/npm
  Browsers:
    Chrome: 80.0.3987.163
    Firefox: 74.0
    Safari: 13.0.5
  npmPackages:
    @apollo/client: ^3.0.0-beta.43 => 3.0.0-beta.43 
    apollo-link-error: ^1.1.12 => 1.1.12 
    apollo-link-token-refresh: ^0.2.7 => 0.2.7 



Hints:
Yeah, can verify it's still happening on 3.0.0-beta.31
Hi, I am also experiencing this issue.

**Versions**
apollo-client: 2.6.8
@apollo/react-hooks: 3.1.5



Same here with @apollo/react-hooks 3.1.5
It is still in `@apollo/client` 3.0.0-beta.44

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
@@ -412,7 +412,13 @@ export class QueryData<TData, TVariables> extends OperationData {
     const { data, loading, error } = this.previousData.result;
 
     if (!loading) {
-      const { query, variables, onCompleted, onError } = this.getOptions();
+      const {
+        query,
+        variables,
+        onCompleted,
+        onError,
+        skip
+      } = this.getOptions();
 
       // No changes, so we won't call onError/onCompleted.
       if (
@@ -424,7 +430,7 @@ export class QueryData<TData, TVariables> extends OperationData {
         return;
       }
 
-      if (onCompleted && !error) {
+      if (onCompleted && !error && !skip) {
         onCompleted(data);
       } else if (onError && error) {
         onError(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
@@ -1798,6 +1798,27 @@ describe('useQuery Hook', () => {
         expect(renderCount).toBe(3);
       }).then(resolve, reject);
     });
+
+    itAsync('should not call onCompleted if skip is true', (resolve, reject) => {
+      function Component() {
+        const { loading } = useQuery(CAR_QUERY, {
+          skip: true,
+          onCompleted() {
+            fail('should not call onCompleted!');
+          }
+        });
+        expect(loading).toBeFalsy();
+        return null;
+      }
+
+      render(
+        <MockedProvider mocks={CAR_MOCKS}>
+          <Component />
+        </MockedProvider>
+      );
+
+      return wait().then(resolve, reject);
+    });
   });
 
   describe('Optimistic data', () => {