From 2d46050d2f773048aa1425208c66d8e2c32428d0 Mon Sep 17 00:00:00 2001
From: hwillson <hugh@octonary.com>
Date: Mon, 13 Jul 2020 14:58:12 -0400
Subject: [PATCH] Ignore option callbacks when deciding to update a query

`QueryData` stores last used options to help decide when it should
re-run. If new options (when compared against the previously
stored last options) are found, `QueryData` will make sure the
new options are passed into Apollo Client for processing.
When `onCompleted` and/or `onError` options are set however,
`QueryData` thinks the options received on each render are new
as these callback functions don't have a stable identity. This
can then lead to infinite re-renders.

This commit adjusts the `QueryData` option equality check to
ignore option callbacks. During normal use of `useQuery` it
should be okay to ignore callbacks like this, as they don't
normally change between renders.

Fixes #6301
---
 src/react/data/QueryData.ts                 | 44 ++++++++++++++++++---
 src/react/hooks/__tests__/useQuery.test.tsx | 37 +++++++++++++++++
 2 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts
index 4d3ad689830..fb558930101 100644
--- a/src/react/data/QueryData.ts
+++ b/src/react/data/QueryData.ts
@@ -214,6 +214,7 @@ export class QueryData<TData, TVariables> extends OperationData {
         ...observableQueryOptions,
         children: null
       };
+
       this.currentObservable = this.refreshClient().client.watchQuery({
         ...observableQueryOptions
       });
@@ -239,12 +240,23 @@ export class QueryData<TData, TVariables> extends OperationData {
       children: null
     };
 
-    if (
-      !equal(
-        newObservableQueryOptions,
-        this.previousData.observableQueryOptions
-      )
-    ) {
+    // if (
+    //   !equal(
+    //     newObservableQueryOptions,
+    //     this.previousData.observableQueryOptions
+    //   )
+    // ) {
+    //   this.previousData.observableQueryOptions = newObservableQueryOptions;
+    //   this.currentObservable
+    //     .setOptions(newObservableQueryOptions)
+    //     // The error will be passed to the child container, so we don't
+    //     // need to log it here. We could conceivably log something if
+    //     // an option was set. OTOH we don't log errors w/ the original
+    //     // query. See https://github.com/apollostack/react-apollo/issues/404
+    //     .catch(() => {});
+    // }
+
+    if (this.haveOptionsChanged(newObservableQueryOptions)) {
       this.previousData.observableQueryOptions = newObservableQueryOptions;
       this.currentObservable
         .setOptions(newObservableQueryOptions)
@@ -484,4 +496,24 @@ export class QueryData<TData, TVariables> extends OperationData {
       subscribeToMore: this.obsSubscribeToMore
     } as ObservableQueryFields<TData, TVariables>;
   }
+
+  private haveOptionsChanged(options: QueryDataOptions<TData, TVariables>) {
+    // When comparing new options against previously stored options,
+    // we'll ignore callback functions since their identities are not
+    // stable, meaning they'll always show as being different. Ignoring
+    // them when determining if options have changed is okay however, as
+    // callback functions are not normally changed between renders.
+    return !equal(
+      {
+        ...options,
+        onCompleted: undefined,
+        onError: undefined
+      },
+      {
+        ...this.previousData.observableQueryOptions,
+        onCompleted: undefined,
+        onError: undefined
+      }
+    );
+  }
 }
diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx
index 9ec841088e5..1787d437494 100644
--- a/src/react/hooks/__tests__/useQuery.test.tsx
+++ b/src/react/hooks/__tests__/useQuery.test.tsx
@@ -1798,6 +1798,43 @@ describe('useQuery Hook', () => {
         expect(renderCount).toBe(3);
       }).then(resolve, reject);
     });
+
+    itAsync(
+      'should not make extra network requests when `onCompleted` is ' +
+      'defined with a `network-only` fetch policy',
+      (resolve, reject) => {
+        let renderCount = 0;
+        function Component() {
+          const { loading, data } = useQuery(CAR_QUERY, {
+            fetchPolicy: 'network-only',
+            onCompleted: () => undefined
+          });
+          switch (++renderCount) {
+            case 1:
+              expect(loading).toBeTruthy();
+              break;
+            case 2:
+              expect(loading).toBeFalsy();
+              expect(data).toEqual(CAR_RESULT_DATA);
+              break;
+            case 3:
+              fail('Too many renders');
+            default:
+          }
+          return null;
+        }
+
+        render(
+          <MockedProvider mocks={CAR_MOCKS}>
+            <Component />
+          </MockedProvider>
+        );
+
+        return wait(() => {
+          expect(renderCount).toBe(2);
+        }).then(resolve, reject);
+      }
+    );
   });
 
   describe('Optimistic data', () => {