From 2353a58eb415221dfaddbb9236c41781580d69c0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 19 Mar 2020 10:53:08 -0700 Subject: [PATCH] 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 --- .../__tests__/ReactCompositeComponent-test.js | 110 ++++++++++++++++++ .../src/ReactFiberBeginWork.js | 2 + .../src/ReactFiberWorkLoop.js | 16 ++- .../src/__tests__/ReactHooks-test.internal.js | 6 +- ...eactHooksWithNoopRenderer-test.internal.js | 2 +- 5 files changed, 126 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 908d418cac7ef..86477cbce0db1 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -1786,4 +1786,114 @@ describe('ReactCompositeComponent', () => { ReactDOM.render(, 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 ( +
+ + +
+ ); + } + const container = document.createElement('div'); + ReactDOM.render(, 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 ( +
+ + +
+ ); + } + const container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, 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 ( +
+ + +
+ ); + } + const container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, 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 ( + + ); + } + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toErrorDev( + 'Cannot update a component (`A`) while rendering a different component (`B`)', + ); + // Dedupe. + ReactDOM.render(, container); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 8eb9f9ffb9502..f576fb7330fe4 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1358,6 +1358,7 @@ function mountIndeterminateComponent( ReactStrictModeWarnings.recordLegacyContextWarning(workInProgress, null); } + setIsRendering(true); ReactCurrentOwner.current = workInProgress; value = renderWithHooks( null, @@ -1367,6 +1368,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 index f65f1ba15a418..e04e6e21f0bd6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -156,6 +156,7 @@ import { import getComponentName from 'shared/getComponentName'; import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import { + isRendering as ReactCurrentDebugFiberIsRenderingInDEV, phase as ReactCurrentDebugFiberPhaseInDEV, resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, @@ -2801,7 +2802,10 @@ if (__DEV__) { function warnAboutRenderPhaseUpdatesInDEV(fiber) { if (__DEV__) { - if ((executionContext & RenderContext) !== NoContext) { + if ( + ReactCurrentDebugFiberIsRenderingInDEV && + (executionContext & RenderContext) !== NoContext + ) { switch (fiber.tag) { case FunctionComponent: case ForwardRef: @@ -2809,14 +2813,14 @@ function warnAboutRenderPhaseUpdatesInDEV(fiber) { 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, diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 6de3567163a3b..c1d0ea18f3f26 100644 --- 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 index 3739e655069c0..bb14e5f7b1c98 100644 --- 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`', ]); });