Don't fire the render phase update warning for class lifecycles (#18330)

* Change the warning to not say "function body" This warning is more generic and may happen with class components too. * Dedupe by the rendering component * Don't warn outside of render
facebook · Mar 19, 2020 · da83408 · da83408
1 parent 7554ab7
commit da83408
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 15 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
@@ -1749,4 +1749,114 @@ describe('ReactCompositeComponent', () => {
     ReactDOM.render(<Shadow />, container);
     expect(container.firstChild.tagName).toBe('DIV');
   });
+
+  it('should not warn on updating function component from componentWillMount', () => {
+    let _setState;
+    function A() {
+      _setState = React.useState()[1];
+      return null;
+    }
+    class B extends React.Component {
+      UNSAFE_componentWillMount() {
+        _setState({});
+      }
+      render() {
+        return null;
+      }
+    }
+    function Parent() {
+      return (
+        <div>
+          <A />
+          <B />
+        </div>
+      );
+    }
+    const container = document.createElement('div');
+    ReactDOM.render(<Parent />, container);
+  });
+
+  it('should not warn on updating function component from componentWillUpdate', () => {
+    let _setState;
+    function A() {
+      _setState = React.useState()[1];
+      return null;
+    }
+    class B extends React.Component {
+      UNSAFE_componentWillUpdate() {
+        _setState({});
+      }
+      render() {
+        return null;
+      }
+    }
+    function Parent() {
+      return (
+        <div>
+          <A />
+          <B />
+        </div>
+      );
+    }
+    const container = document.createElement('div');
+    ReactDOM.render(<Parent />, container);
+    ReactDOM.render(<Parent />, container);
+  });
+
+  it('should not warn on updating function component from componentWillReceiveProps', () => {
+    let _setState;
+    function A() {
+      _setState = React.useState()[1];
+      return null;
+    }
+    class B extends React.Component {
+      UNSAFE_componentWillReceiveProps() {
+        _setState({});
+      }
+      render() {
+        return null;
+      }
+    }
+    function Parent() {
+      return (
+        <div>
+          <A />
+          <B />
+        </div>
+      );
+    }
+    const container = document.createElement('div');
+    ReactDOM.render(<Parent />, container);
+    ReactDOM.render(<Parent />, container);
+  });
+
+  it('should warn on updating function component from render', () => {
+    let _setState;
+    function A() {
+      _setState = React.useState()[1];
+      return null;
+    }
+    class B extends React.Component {
+      render() {
+        _setState({});
+        return null;
+      }
+    }
+    function Parent() {
+      return (
+        <div>
+          <A />
+          <B />
+        </div>
+      );
+    }
+    const container = document.createElement('div');
+    expect(() => {
+      ReactDOM.render(<Parent />, container);
+    }).toErrorDev(
+      'Cannot update a component (`A`) while rendering a different component (`B`)',
+    );
+    // Dedupe.
+    ReactDOM.render(<Parent />, container);
+  });
 });
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -1356,6 +1356,7 @@ function mountIndeterminateComponent(
       ReactStrictModeWarnings.recordLegacyContextWarning(workInProgress, null);
     }
 
+    setIsRendering(true);
     ReactCurrentOwner.current = workInProgress;
     value = renderWithHooks(
       null,
@@ -1365,6 +1366,7 @@ function mountIndeterminateComponent(
       context,
       renderExpirationTime,
     );
+    setIsRendering(false);
   } else {
     value = renderWithHooks(
       null,

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -2800,22 +2800,25 @@ if (__DEV__) {
 
 function warnAboutRenderPhaseUpdatesInDEV(fiber) {
   if (__DEV__) {
-    if ((executionContext & RenderContext) !== NoContext) {
+    if (
+      ReactCurrentDebugFiberIsRenderingInDEV &&
+      (executionContext & RenderContext) !== NoContext
+    ) {
       switch (fiber.tag) {
         case FunctionComponent:
         case ForwardRef:
         case SimpleMemoComponent: {
           const renderingComponentName =
             (workInProgress && getComponentName(workInProgress.type)) ||
             'Unknown';
-          const setStateComponentName =
-            getComponentName(fiber.type) || 'Unknown';
-          const dedupeKey =
-            renderingComponentName + ' ' + setStateComponentName;
+          // Dedupe by the rendering component because it's the one that needs to be fixed.
+          const dedupeKey = renderingComponentName;
           if (!didWarnAboutUpdateInRenderForAnotherComponent.has(dedupeKey)) {
             didWarnAboutUpdateInRenderForAnotherComponent.add(dedupeKey);
+            const setStateComponentName =
+              getComponentName(fiber.type) || 'Unknown';
             console.error(
-              'Cannot update a component (`%s`) from inside the function body of a ' +
+              'Cannot update a component (`%s`) while rendering a ' +
                 'different component (`%s`). To locate the bad setState() call inside `%s`, ' +
                 'follow the stack trace as described in https://fb.me/setstate-in-render',
               setStateComponentName,
@@ -2826,18 +2829,15 @@ function warnAboutRenderPhaseUpdatesInDEV(fiber) {
           break;
         }
         case ClassComponent: {
-          if (
-            ReactCurrentDebugFiberIsRenderingInDEV &&
-            !didWarnAboutUpdateInRender
-          ) {
+          if (!didWarnAboutUpdateInRender) {
             console.error(
               'Cannot update during an existing state transition (such as ' +
                 'within `render`). Render methods should be a pure ' +
                 'function of props and state.',
             );
             didWarnAboutUpdateInRender = true;
-            break;
           }
+          break;
         }
       }
     }

diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
@@ -1087,7 +1087,7 @@ describe('ReactHooks', () => {
       ),
     ).toErrorDev([
       'Context can only be read while React is rendering',
-      'Cannot update a component (`Fn`) from inside the function body of a different component (`Cls`).',
+      'Cannot update a component (`Fn`) while rendering a different component (`Cls`).',
     ]);
   });
 
@@ -1783,8 +1783,8 @@ describe('ReactHooks', () => {
     if (__DEV__) {
       expect(console.error).toHaveBeenCalledTimes(2);
       expect(console.error.calls.argsFor(0)[0]).toContain(
-        'Warning: Cannot update a component (`%s`) from inside the function body ' +
-          'of a different component (`%s`).',
+        'Warning: Cannot update a component (`%s`) while rendering ' +
+          'a different component (`%s`).',
       );
     }
   });

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -458,7 +458,7 @@ function loadModules({
             expect(() =>
               expect(Scheduler).toFlushAndYield(['Foo [0]', 'Bar', 'Foo [1]']),
             ).toErrorDev([
-              'Cannot update a component (`Foo`) from inside the function body of a ' +
+              'Cannot update a component (`Foo`) while rendering a ' +
                 'different component (`Bar`). To locate the bad setState() call inside `Bar`',
             ]);
           });