From 2b6ac39ecc624130385a46f9cf293e6c07a948da 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                 | 17 ++++++----
 src/react/hooks/__tests__/useQuery.test.tsx | 37 +++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts
index 4d3ad689830..d311b0c05ea 100644
--- a/src/react/data/QueryData.ts
+++ b/src/react/data/QueryData.ts
@@ -27,6 +27,8 @@ import {
 } from '../types/types';
 import { OperationData } from './OperationData';
 
+const stripFns = ({ onCompleted, onError, ...rest }: any = {}) => rest;
+
 export class QueryData<TData, TVariables> extends OperationData {
   public onNewData: () => void;
 
@@ -239,12 +241,15 @@ export class QueryData<TData, TVariables> extends OperationData {
       children: null
     };
 
-    if (
-      !equal(
-        newObservableQueryOptions,
-        this.previousData.observableQueryOptions
-      )
-    ) {
+    // 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.
+    if (!equal(
+      stripFns(newObservableQueryOptions),
+      stripFns(this.previousData.observableQueryOptions),
+    )) {
       this.previousData.observableQueryOptions = newObservableQueryOptions;
       this.currentObservable
         .setOptions(newObservableQueryOptions)
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', () => {