Dedupe onError and ensure refetch sets loading state properly

Changelog.md

@@ -9,6 +9,8 @@
 
 ### Bug Fixes
 
+- Dedupe `onError` callback calls and ensure `refetch` sets `loading` state properly. <br/>
+  [@hwillson](https://github.com/hwillson) in [#3339](https://github.com/apollographql/react-apollo/pull/3339)
 - Add missing `useLazyQuery` export to the `react-apollo` (all) package. <br/>
   [@hwillson](https://github.com/hwillson) in [#3320](https://github.com/apollographql/react-apollo/pull/3320)
 - Remove `void` from being one of the `MutationTuple` mutate function possible generics. This will make it easier to properly destructure results returned by the mutate function Promise. <br/>

examples/hooks/client/package.json

@@ -9,7 +9,7 @@
     "@types/react": "16.8.24",
     "@types/react-dom": "16.8.5",
     "apollo-cache-inmemory": "^1.6.0",
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "apollo-link-http": "^1.5.14",
     "apollo-link-ws": "^1.0.17",
     "graphql": "^14.3.1",

package.json

@@ -10,11 +10,11 @@
     "test": "npx jest --config ./config/jest.config.js --silent",
     "test:watch": "npx jest --config ./config/jest.config.js --watch",
     "test:coverage": "npx jest --config ./config/jest.config.js --verbose --coverage",
-    "test:ci": "npm run test:coverage -- --ci --maxWorkers=2 --reporters=default --reporters=jest-junit",
+    "test:ci": "npm run test:coverage -- --ci --maxWorkers=2 --reporters=default --reporters=jest-junit --silent",
     "test:cjs": "npm run build && npx jest --config ./config/jest.cjs.config.js",
-    "test:cjs:ci": "npx jest --config ./config/jest.cjs.config.js --ci --maxWorkers=2 --reporters=default --reporters=jest-junit",
+    "test:cjs:ci": "npx jest --config ./config/jest.cjs.config.js --ci --maxWorkers=2 --reporters=default --reporters=jest-junit --silent",
     "test:umd": "npm run build && npx jest --config ./config/jest.umd.config.js",
-    "test:umd:ci": "npx jest --config ./config/jest.umd.config.js --ci --maxWorkers=2 --reporters=default --reporters=jest-junit",
+    "test:umd:ci": "npx jest --config ./config/jest.umd.config.js --ci --maxWorkers=2 --reporters=default --reporters=jest-junit --silent",
     "bundlesize": "npx lerna run build && bundlesize",
     "prettier": "npx prettier --config ./config/prettier.config.js --write \"./**/*.{js,jsx,ts*,md,graphql,json}\"",
     "deploy": "npx lerna publish",
@@ -46,7 +46,7 @@
     "@types/zen-observable": "0.8.0",
     "apollo-cache": "1.3.2",
     "apollo-cache-inmemory": "1.6.2",
-    "apollo-client": "2.6.3",
+    "apollo-client": "2.6.4",
     "apollo-link": "1.2.12",
     "bundlesize": "0.18.0",
     "graphql": "14.4.2",

packages/all/package.json

@@ -33,7 +33,7 @@
     "deploy": "npm publish"
   },
   "peerDependencies": {
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "graphql": "^14.3.1",
     "react": "^16.8.0",
     "react-dom": "^16.8.0"

packages/common/package.json

@@ -32,7 +32,7 @@
     "test:umd": "npm run build && npx jest --config ../../config/jest.umd.config.js --testPathPattern packages/common"
   },
   "peerDependencies": {
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "apollo-utilities": "^1.3.2",
     "graphql": "^14.3.1",
     "react": "^16.8.0"

packages/components/package.json

@@ -37,7 +37,7 @@
   },
   "peerDependencies": {
     "apollo-cache": "^1.3.2",
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "apollo-link": "^1.2.12",
     "apollo-utilities": "^1.3.2",
     "graphql": "^14.3.1",

packages/components/src/__tests__/client/Query.test.tsx

@@ -1524,14 +1524,11 @@ describe('Query component', () => {
                     });
                   }, 0);
                   break;
-                // Further fix required in QueryManager, we should have an extra
-                // step for the loading status of the third result
-                // case 4:
-                //   expect(result.loading).toBeTruthy();
-                //   expect(result.error).toBeFalsy();
-                //   break;
                 case 4:
-                  // Third result's data is loaded
+                  expect(result.loading).toBeTruthy();
+                  expect(result.error).toBeFalsy();
+                  break;
+                case 5:
                   expect(result.loading).toBeFalsy();
                   expect(result.error).toBeFalsy();
                   if (!result.data) {

packages/hoc/package.json

@@ -36,7 +36,7 @@
     "test:umd": "npm run build && npx jest --config ../../config/jest.umd.config.js --testPathPattern packages/hoc"
   },
   "peerDependencies": {
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "graphql": "^14.3.1",
     "react": "^16.8.0",
     "react-dom": "^16.8.0"

packages/hoc/src/__tests__/__snapshots__/MockedProvider.test.tsx.snap

@@ -24,16 +24,6 @@ exports[`General use errors if the query in the mock and component do not match
 , variables: {"username":"mock_username"}]
 `;
 
-exports[`General use errors if the query in the mock and component do not match 2`] = `
-[Error: Network error: No more mocked responses for the query: query GetUser($username: String!) {
-  user(username: $username) {
-    id
-    __typename
-  }
-}
-, variables: {"username":"mock_username"}]
-`;
-
 exports[`General use errors if the variables do not deep equal 1`] = `
 [Error: Network error: No more mocked responses for the query: query GetUser($username: String!) {
   user(username: $username) {
@@ -44,16 +34,6 @@ exports[`General use errors if the variables do not deep equal 1`] = `
 , variables: {"username":"some_user","age":42}]
 `;
 
-exports[`General use errors if the variables do not deep equal 2`] = `
-[Error: Network error: No more mocked responses for the query: query GetUser($username: String!) {
-  user(username: $username) {
-    id
-    __typename
-  }
-}
-, variables: {"username":"some_user","age":42}]
-`;
-
 exports[`General use errors if the variables in the mock and component do not match 1`] = `
 [Error: Network error: No more mocked responses for the query: query GetUser($username: String!) {
   user(username: $username) {
@@ -64,16 +44,6 @@ exports[`General use errors if the variables in the mock and component do not ma
 , variables: {"username":"other_user"}]
 `;
 
-exports[`General use errors if the variables in the mock and component do not match 2`] = `
-[Error: Network error: No more mocked responses for the query: query GetUser($username: String!) {
-  user(username: $username) {
-    id
-    __typename
-  }
-}
-, variables: {"username":"other_user"}]
-`;
-
 exports[`General use mocks the data 1`] = `
 Object {
   "__typename": "User",

packages/hoc/src/__tests__/queries/errors.test.tsx

@@ -428,6 +428,9 @@ describe('[queries] errors', () => {
                   });
                 break;
               case 3:
+                expect(props.data!.loading).toBeTruthy();
+                break;
+              case 4:
                 expect(props.data!.loading).toBeFalsy();
                 expect(props.data!.error).toBeFalsy();
                 expect(stripSymbols(props.data!.allPeople)).toEqual(
@@ -621,7 +624,11 @@ describe('[queries] errors', () => {
               }, 50);
               break;
             case 2:
+              expect(this.props.data!.loading).toBe(true);
+              break;
+            case 3:
               expect(this.props.data!.loading).toBe(false);
+              expect(this.props.data!.allPeople).toEqual(dataTwo.allPeople);
               done();
               break;
             default:

packages/hooks/package.json

@@ -36,7 +36,7 @@
     "test:umd": "npm run build && npx jest --config ../../config/jest.umd.config.js --testPathPattern packages/hooks"
   },
   "peerDependencies": {
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "graphql": "^14.3.1",
     "react": "^16.8.0",
     "react-dom": "^16.8.0"

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

@@ -3,7 +3,10 @@ import { DocumentNode, GraphQLError } from 'graphql';
 import gql from 'graphql-tag';
 import { MockedProvider, MockLink } from '@apollo/react-testing';
 import { render, cleanup } from '@testing-library/react';
-import { useQuery } from '@apollo/react-hooks';
+import { useQuery, ApolloProvider } from '@apollo/react-hooks';
+import { ApolloClient } from 'apollo-client';
+import { ApolloLink, Observable } from 'apollo-link';
+import { InMemoryCache } from 'apollo-cache-inmemory';
 
 describe('useQuery Hook', () => {
   const CAR_QUERY: DocumentNode = gql`
@@ -239,5 +242,82 @@ describe('useQuery Hook', () => {
         </MockedProvider>
       );
     });
+
+    it('should only call onError callbacks once', done => {
+      const query = gql`
+        query SomeQuery {
+          stuff {
+            thing
+          }
+        }
+      `;
+
+      const resultData = { stuff: { thing: 'it!', __typename: 'Stuff' } };
+
+      let callCount = 0;
+      const link = new ApolloLink(() => {
+        if (!callCount) {
+          callCount += 1;
+          return new Observable(observer => {
+            observer.error(new Error('Oh no!'));
+          });
+        } else {
+          return Observable.of({ data: resultData });
+        }
+      });
+
+      const client = new ApolloClient({
+        link,
+        cache: new InMemoryCache()
+      });
+
+      const onErrorMock = jest.fn();
+
+      let renderCount = 0;
+      const Component = () => {
+        const { loading, error, refetch, data, networkStatus } = useQuery(
+          query,
+          {
+            onError: onErrorMock,
+            notifyOnNetworkStatusChange: true
+          }
+        );
+
+        console.log('ns', networkStatus);
+
+        switch (renderCount) {
+          case 0:
+            expect(loading).toBeTruthy();
+            break;
+          case 1:
+            expect(loading).toBeFalsy();
+            expect(error).toBeDefined();
+            expect(error!.message).toEqual('Network error: Oh no!');
+            setTimeout(() => {
+              expect(onErrorMock.mock.calls.length).toBe(1);
+              refetch();
+            });
+            break;
+          case 2:
+            expect(loading).toBeTruthy();
+            break;
+          case 3:
+            expect(loading).toBeFalsy();
+            expect(data).toEqual(resultData);
+            done();
+            break;
+          default: // Do nothing
+        }
+
+        renderCount += 1;
+        return null;
+      };
+
+      render(
+        <ApolloProvider client={client}>
+          <Component />
+        </ApolloProvider>
+      );
+    });
   });
 });

packages/hooks/src/data/QueryData.ts

@@ -285,7 +285,10 @@ export class QueryData<TData, TVariables> extends OperationData {
       error: error => {
         this.resubscribeToQuery();
         if (!error.hasOwnProperty('graphQLErrors')) throw error;
-        this.forceUpdate();
+        if (!isEqual(error, this.previousData.error)) {
+          this.previousData.error = error;
+          this.forceUpdate();
+        }
       }
     });
   }

packages/hooks/src/types.ts

@@ -2,7 +2,8 @@ import { ReactNode } from 'react';
 import {
   ApolloClient,
   ApolloQueryResult,
-  ObservableQuery
+  ObservableQuery,
+  ApolloError
 } from 'apollo-client';
 import { Observable } from 'apollo-link';
 import {
@@ -52,6 +53,7 @@ export interface QueryPreviousData<TData, TVariables> {
   result?: ApolloQueryResult<TData> | null;
   loading?: boolean;
   options?: QueryOptions<TData, TVariables>;
+  error?: ApolloError;
 }
 
 export interface QueryCurrentObservable<TData, TVariables> {

packages/hooks/src/utils/useBaseQuery.ts

@@ -34,6 +34,7 @@ export function useBaseQuery<TData = any, TVariables = OperationVariables>(
     context,
     tick
   };
+
   const result = useDeepMemo(
     () => (lazy ? queryData.executeLazy() : queryData.execute()),
     memo

packages/testing/package.json

@@ -33,7 +33,7 @@
   },
   "peerDependencies": {
     "apollo-cache-inmemory": "^1.6.2",
-    "apollo-client": "^2.6.3",
+    "apollo-client": "^2.6.4",
     "apollo-link": "^1.2.12",
     "apollo-utilities": "^1.3.2",
     "graphql": "^14.3.1",