Prevent new data re-render attempts during an existing render (#3902)

* Dependency updates * Use more precise new data callback naming; add a small SSR helper This commit includes a few small `QueryData` adjustments: - Replace the `forceUpdate` callback property name with `onNewData`, to make its use more clear. - Create a small public "has SSR started" helper utility that can be used by things like the `useQuery` hook to change its behavior depending on how it's being used. * Prevent new data re-render attempts during an existing render As of version 16.13.0, React logs a warning when a function component is updated during another component's render phase (facebook/react#17099). In React Apollo, this warning can be triggered when nesting multiple components that leverage `useQuery`. To help avoid this, this commit ensures re-render requests to show new data are delayed until an effect hook is run to handle them (since we're then out of the render phase). * Update react types and `useReducer` calls to skip initial value The latest `useReducer` API doesn't require an initial value. * Changelog update * Bundlesize increase * Code review tweaks * Remove 3.1.4 date
apollographql · Apr 3, 2020 · 678556b · 678556b
1 parent 07e404b
commit 678556b
Show file tree

Hide file tree

Showing 13 changed files with 11,661 additions and 7,664 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -1,5 +1,10 @@
 # Change log
 
+## 3.1.4 (TBD)
+
+- Prevent new data re-render attempts during an existing render. This helps avoid React 16.13.0's "Cannot update a component from inside the function body of a different component" warning (https://github.com/facebook/react/pull/17099). <br/>
+  [@hwillson](https://github.com/hwillson) in [#3902](https://github.com/apollographql/react-apollo/pull/3902)
+
 ## 3.1.3 (2019-10-15)
 
 - Revert the changes made in [#3497](https://github.com/apollographql/react-apollo/pull/3497), which have lead to problems with `onCompleted` being called more often than necessary. <br/>

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -40,22 +40,23 @@
     "@types/invariant": "2.2.30",
     "@types/jest": "24.0.18",
     "@types/prop-types": "15.7.3",
-    "@types/react": "16.9.5",
-    "@types/react-dom": "16.9.1",
+    "@types/react": "^16.9.26",
+    "@types/react-dom": "^16.9.5",
     "@types/recompose": "0.30.7",
     "@types/zen-observable": "0.8.0",
     "apollo-cache": "1.3.2",
     "apollo-cache-inmemory": "1.6.3",
-    "apollo-client": "2.6.4",
+    "apollo-client": "^2.6.8",
     "apollo-link": "1.2.13",
     "bundlesize": "0.18.0",
     "graphql": "14.5.8",
     "graphql-tag": "2.10.1",
     "jest": "24.9.0",
     "jest-junit": "8.0.0",
     "lerna": "3.17.0",
-    "react": "16.10.2",
-    "react-dom": "16.10.2",
+    "prop-types": "^15.7.2",
+    "react": "^16.13.1",
+    "react-dom": "^16.13.1",
     "recompose": "0.30.0",
     "rollup": "1.23.1",
     "rollup-plugin-commonjs": "10.1.0",
@@ -65,7 +66,7 @@
     "rollup-plugin-typescript2": "0.24.3",
     "ts-jest": "24.1.0",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4",
+    "typescript": "3.8.3",
     "zen-observable-ts": "0.8.20"
   },
   "bundlesize": [
@@ -92,7 +93,7 @@
     {
       "name": "@apollo/react-hooks",
       "path": "./packages/hooks/lib/react-hooks.cjs.min.js",
-      "maxSize": "4.2 kB"
+      "maxSize": "4.3 kB"
     },
     {
       "name": "@apollo/react-ssr",

diff --git a/packages/all/package.json b/packages/all/package.json
@@ -54,6 +54,6 @@
   },
   "devDependencies": {
     "rollup": "1.23.1",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/common/package.json b/packages/common/package.json
@@ -52,6 +52,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/components/package.json b/packages/components/package.json
@@ -63,6 +63,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/hoc/package.json b/packages/hoc/package.json
@@ -60,6 +60,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/hooks/package.json b/packages/hooks/package.json
@@ -58,6 +58,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/hooks/src/__tests__/useQuery.test.tsx b/packages/hooks/src/__tests__/useQuery.test.tsx
@@ -1,4 +1,4 @@
-import React, { useState, useReducer } from 'react';
+import React, { Fragment, useState, useReducer } from 'react';
 import { DocumentNode, GraphQLError } from 'graphql';
 import gql from 'graphql-tag';
 import { MockedProvider, MockLink } from '@apollo/react-testing';
@@ -121,6 +121,61 @@ describe('useQuery Hook', () => {
 
       await wait();
     });
+
+    it('should not error when forcing an update with React >= 16.13.0', async () => {
+      let wasUpdateErrorLogged = false;
+      const consoleError = console.error;
+      console.error = (msg: string) => {
+        console.log(msg);
+        wasUpdateErrorLogged = msg.indexOf('Cannot update a component') > -1;
+      };
+
+      const CAR_MOCKS = [1, 2, 3, 4, 5, 6].map(something => ({
+        request: {
+          query: CAR_QUERY,
+          variables: { something }
+        },
+        result: { data: CAR_RESULT_DATA },
+        delay: 1000
+      }));
+
+      let renderCount = 0;
+
+      const InnerComponent = ({ something }: any) => {
+        const { loading, data } = useQuery(CAR_QUERY, {
+          fetchPolicy: 'network-only',
+          variables: { something }
+        });
+        if (loading) return null;
+        expect(wasUpdateErrorLogged).toBeFalsy();
+        expect(data).toEqual(CAR_RESULT_DATA);
+        renderCount += 1;
+        return null;
+      };
+
+      function WrapperComponent({ something }: any) {
+        const { loading } = useQuery(CAR_QUERY, {
+          variables: { something }
+        });
+        return loading ? null : <InnerComponent something={something + 1} />;
+      }
+
+      render(
+        <MockedProvider mocks={CAR_MOCKS}>
+          <Fragment>
+            <WrapperComponent something={1} />
+            <WrapperComponent something={3} />
+            <WrapperComponent something={5} />
+          </Fragment>
+        </MockedProvider>
+      );
+
+      await wait(() => {
+        expect(renderCount).toBe(3);
+      }).finally(() => {
+        console.error = consoleError;
+      });
+    });
   });
 
   describe('Polling', () => {
@@ -454,7 +509,7 @@ describe('useQuery Hook', () => {
             expect(error).toBeDefined();
             expect(error!.message).toEqual('GraphQL error: forced error');
             setTimeout(() => {
-              forceUpdate(0);
+              forceUpdate();
             });
             break;
           case 2:
@@ -514,7 +569,7 @@ describe('useQuery Hook', () => {
               expect(error).toBeDefined();
               expect(error!.message).toEqual('GraphQL error: forced error');
               setTimeout(() => {
-                forceUpdate(0);
+                forceUpdate();
               });
               break;
             case 2:
@@ -1094,7 +1149,6 @@ describe('useQuery Hook', () => {
             onCompleted(data) {
               onCompletedCalled = true;
               expect(data).toBeDefined();
-              console.log(data);
             }
           });
           if (!loading) {

diff --git a/packages/hooks/src/data/QueryData.ts b/packages/hooks/src/data/QueryData.ts
@@ -25,24 +25,24 @@ import {
 import { OperationData } from './OperationData';
 
 export class QueryData<TData, TVariables> extends OperationData {
+  public onNewData: () => void;
+
   private previousData: QueryPreviousData<TData, TVariables> = {};
   private currentObservable: QueryCurrentObservable<TData, TVariables> = {};
-  private forceUpdate: any;
-
   private runLazy: boolean = false;
   private lazyOptions?: QueryLazyOptions<TVariables>;
 
   constructor({
     options,
     context,
-    forceUpdate
+    onNewData
   }: {
     options: QueryOptions<TData, TVariables>;
     context: ApolloContextValue;
-    forceUpdate: any;
+    onNewData: () => void;
   }) {
     super(options, context);
-    this.forceUpdate = forceUpdate;
+    this.onNewData = onNewData;
   }
 
   public execute(): QueryResult<TData, TVariables> {
@@ -134,12 +134,15 @@ export class QueryData<TData, TVariables> extends OperationData {
     return options;
   }
 
+  public ssrInitiated() {
+    return this.context && this.context.renderPromises;
+  }
+
   private runLazyQuery = (options?: QueryLazyOptions<TVariables>) => {
     this.cleanup();
-
     this.runLazy = true;
     this.lazyOptions = options;
-    this.forceUpdate();
+    this.onNewData();
   };
 
   private getExecuteResult = (): QueryResult<TData, TVariables> => {
@@ -149,7 +152,6 @@ export class QueryData<TData, TVariables> extends OperationData {
   };
 
   private getExecuteSsrResult() {
-    const treeRenderingInitiated = this.context && this.context.renderPromises;
     const ssrDisabled = this.getOptions().ssr === false;
     const fetchDisabled = this.refreshClient().client.disableNetworkFetches;
 
@@ -162,12 +164,12 @@ export class QueryData<TData, TVariables> extends OperationData {
 
     // If SSR has been explicitly disabled, and this function has been called
     // on the server side, return the default loading state.
-    if (ssrDisabled && (treeRenderingInitiated || fetchDisabled)) {
+    if (ssrDisabled && (this.ssrInitiated() || fetchDisabled)) {
       return ssrLoading;
     }
 
     let result;
-    if (treeRenderingInitiated) {
+    if (this.ssrInitiated()) {
       result =
         this.context.renderPromises!.addQueryPromise(
           this,
@@ -186,8 +188,7 @@ export class QueryData<TData, TVariables> extends OperationData {
     // Set the fetchPolicy to cache-first for network-only and cache-and-network
     // fetches for server side renders.
     if (
-      this.context &&
-      this.context.renderPromises &&
+      this.ssrInitiated() &&
       (options.fetchPolicy === 'network-only' ||
         options.fetchPolicy === 'cache-and-network')
     ) {
@@ -206,8 +207,8 @@ export class QueryData<TData, TVariables> extends OperationData {
     // See if there is an existing observable that was used to fetch the same
     // data and if so, use it instead since it will contain the proper queryId
     // to fetch the result set. This is used during SSR.
-    if (this.context && this.context.renderPromises) {
-      this.currentObservable.query = this.context.renderPromises.getSSRObservable(
+    if (this.ssrInitiated()) {
+      this.currentObservable.query = this.context!.renderPromises!.getSSRObservable(
         this.getOptions()
       );
     }
@@ -223,8 +224,8 @@ export class QueryData<TData, TVariables> extends OperationData {
         ...observableQueryOptions
       });
 
-      if (this.context && this.context.renderPromises) {
-        this.context.renderPromises.registerSSRObservable(
+      if (this.ssrInitiated()) {
+        this.context?.renderPromises?.registerSSRObservable(
           this.currentObservable.query,
           observableQueryOptions
         );
@@ -279,7 +280,7 @@ export class QueryData<TData, TVariables> extends OperationData {
           return;
         }
 
-        this.forceUpdate();
+        this.onNewData();
       },
       error: error => {
         this.resubscribeToQuery();
@@ -291,7 +292,7 @@ export class QueryData<TData, TVariables> extends OperationData {
           !isEqual(error, this.previousData.error)
         ) {
           this.previousData.error = error;
-          this.forceUpdate();
+          this.onNewData();
         }
       }
     });

diff --git a/packages/hooks/src/utils/useBaseQuery.ts b/packages/hooks/src/utils/useBaseQuery.ts
@@ -18,21 +18,39 @@ export function useBaseQuery<TData = any, TVariables = OperationVariables>(
   const context = useContext(getApolloContext());
   const [tick, forceUpdate] = useReducer(x => x + 1, 0);
   const updatedOptions = options ? { ...options, query } : { query };
+  const isRendering = useRef(true);
+  const isRenderScheduled = useRef(false);
 
   const queryDataRef = useRef<QueryData<TData, TVariables>>();
-
-  if (!queryDataRef.current) {
-    queryDataRef.current = new QueryData<TData, TVariables>({
+  const queryData =
+    queryDataRef.current ||
+    new QueryData<TData, TVariables>({
       options: updatedOptions as QueryOptions<TData, TVariables>,
       context,
-      forceUpdate
+      onNewData() {
+        // When new data is received from the `QueryData` object, we want to
+        // force a re-render to make sure the new data is displayed. We can't
+        // force that re-render if we're already rendering however, so in that
+        // case we'll defer triggering a re-render until we're inside an effect
+        // hook.
+        if (!queryData.ssrInitiated() && isRendering.current) {
+          isRenderScheduled.current = true;
+        } else {
+          forceUpdate();
+        }
+      }
     });
-  }
 
-  const queryData = queryDataRef.current;
   queryData.setOptions(updatedOptions);
   queryData.context = context;
 
+  // SSR won't trigger the effect hook below that stores the current
+  // `QueryData` instance for future renders, so we'll handle that here if
+  // the current render is happening server side.
+  if (queryData.ssrInitiated() && !queryDataRef.current) {
+    queryDataRef.current = queryData;
+  }
+
   // `onError` and `onCompleted` callback functions will not always have a
   // stable identity, so we'll exclude them from the memoization key to
   // prevent `afterExecute` from being triggered un-necessarily.
@@ -51,6 +69,23 @@ export function useBaseQuery<TData = any, TVariables = OperationVariables>(
     ? (result as QueryTuple<TData, TVariables>)[1]
     : (result as QueryResult<TData, TVariables>);
 
+  useEffect(() => {
+    // We only need one instance of the `QueryData` class, so we'll store it
+    // as a ref to make it available on subsequent renders.
+    if (!queryDataRef.current) {
+      queryDataRef.current = queryData;
+    }
+
+    // If `QueryData` requested a re-render to show new data while we were
+    // in a render phase, let's handle the re-render here where it's safe to do
+    // so.
+    isRendering.current = false;
+    if (isRenderScheduled.current) {
+      isRenderScheduled.current = false;
+      forceUpdate();
+    }
+  });
+
   useEffect(() => queryData.afterExecute({ lazy }), [
     queryResult.loading,
     queryResult.networkStatus,

diff --git a/packages/ssr/package.json b/packages/ssr/package.json
@@ -54,6 +54,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }
diff --git a/packages/testing/package.json b/packages/testing/package.json
@@ -54,6 +54,6 @@
     "jest": "24.9.0",
     "rollup": "1.23.1",
     "tsc-watch": "3.0.2",
-    "typescript": "3.6.4"
+    "typescript": "3.8.3"
   }
 }