From 1051db9b65a4897a5bf91c11a36d048e3b6da715 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 15 Oct 2019 14:15:18 -0700 Subject: [PATCH] Warn for update on different component in render This warning already exists for class components, but not for functions. It does not apply to render phase updates to the same component, which have special semantics that we do support. --- .../src/ReactFiberWorkLoop.js | 52 ++++++++++++------- .../src/__tests__/ReactHooks-test.internal.js | 31 +++++++---- ...eactHooksWithNoopRenderer-test.internal.js | 44 ++++++++++++++++ 3 files changed, 98 insertions(+), 29 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index e2f71c7a4cb1..31946efd199b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -380,7 +380,7 @@ export function scheduleUpdateOnFiber( expirationTime: ExpirationTime, ) { checkForNestedUpdates(); - warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber); + warnAboutRenderPhaseUpdatesInDEV(fiber); const root = markUpdateTimeFromFiberToRoot(fiber, expirationTime); if (root === null) { @@ -2781,30 +2781,44 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { let didWarnAboutUpdateInRender = false; let didWarnAboutUpdateInGetChildContext = false; -function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) { +function warnAboutRenderPhaseUpdatesInDEV(fiber) { if (__DEV__) { - if (fiber.tag === ClassComponent) { - switch (ReactCurrentDebugFiberPhaseInDEV) { - case 'getChildContext': - if (didWarnAboutUpdateInGetChildContext) { - return; - } + if ((executionContext & RenderContext) !== NoContext) { + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { console.error( - 'setState(...): Cannot call setState() inside getChildContext()', + 'Cannot update a component from inside the function body of a ' + + 'different component.', ); - didWarnAboutUpdateInGetChildContext = true; break; - case 'render': - if (didWarnAboutUpdateInRender) { - return; + } + case ClassComponent: { + switch (ReactCurrentDebugFiberPhaseInDEV) { + case 'getChildContext': + if (didWarnAboutUpdateInGetChildContext) { + return; + } + console.error( + 'setState(...): Cannot call setState() inside getChildContext()', + ); + didWarnAboutUpdateInGetChildContext = true; + break; + case 'render': + if (didWarnAboutUpdateInRender) { + return; + } + 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; } - 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; + } } } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 2ebf8d16a85e..f9073f1bf701 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1085,7 +1085,10 @@ describe('ReactHooks', () => { , ), - ).toErrorDev(['Context can only be read while React is rendering']); + ).toErrorDev([ + 'Context can only be read while React is rendering', + 'Cannot update a component from inside the function body of a different component.', + ]); }); it('warns when calling hooks inside useReducer', () => { @@ -1749,8 +1752,9 @@ describe('ReactHooks', () => { }); // Regression test for #14674 - it('does not swallow original error when updating another component in render phase', () => { + it('does not swallow original error when updating another component in render phase', async () => { let {useState} = React; + spyOnDev(console, 'error'); let _setState; function A() { @@ -1760,22 +1764,29 @@ describe('ReactHooks', () => { } function B() { - act(() => - _setState(() => { - throw new Error('Hello'); - }), - ); + _setState(() => { + throw new Error('Hello'); + }); return null; } - expect(() => + await act(async () => { ReactTestRenderer.create( <> , - ), - ).toThrow('Hello'); + ); + expect(() => Scheduler.unstable_flushAll()).toThrow('Hello'); + }); + + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(2); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Cannot update a component from inside the function body ' + + 'of a different component.%s', + ); + } }); // Regression test for https://github.com/facebook/react/issues/15057 diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 6e50579addd3..325d5ea7e7e0 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -420,6 +420,50 @@ function loadModules({ ]); }); + it('warns about render phase update on a different component', async () => { + let setStep; + function Foo() { + const [step, _setStep] = useState(0); + setStep = _setStep; + return ; + } + + function Bar({triggerUpdate}) { + if (triggerUpdate) { + setStep(1); + } + return ; + } + + const root = ReactNoop.createRoot(); + + await ReactNoop.act(async () => { + root.render( + <> + + + , + ); + }); + expect(Scheduler).toHaveYielded(['Foo [0]', 'Bar']); + + // Bar will update Foo during its render phase. React should warn. + await ReactNoop.act(async () => { + root.render( + <> + + + , + ); + expect(() => + expect(Scheduler).toFlushAndYield(['Foo [0]', 'Bar', 'Foo [1]']), + ).toErrorDev([ + 'Cannot update a component from inside the function body of a ' + + 'different component.', + ]); + }); + }); + it('keeps restarting until there are no more new updates', () => { function Counter({row: newRow}) { let [count, setCount] = useState(0);