Skip to content

Commit

Permalink
Warn for update on different component in render (#17099)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Feb 19, 2020
1 parent 085d021 commit ea6ed3d
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 29 deletions.
52 changes: 33 additions & 19 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ export function scheduleUpdateOnFiber(
expirationTime: ExpirationTime,
) {
checkForNestedUpdates();
warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber);
warnAboutRenderPhaseUpdatesInDEV(fiber);

const root = markUpdateTimeFromFiberToRoot(fiber, expirationTime);
if (root === null) {
Expand Down Expand Up @@ -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;
}
}
}
}
Expand Down
31 changes: 21 additions & 10 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,10 @@ describe('ReactHooks', () => {
<Cls />
</>,
),
).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', () => {
Expand Down Expand Up @@ -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() {
Expand All @@ -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(
<>
<A />
<B />
</>,
),
).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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Text text={`Foo [${step}]`} />;
}

function Bar({triggerUpdate}) {
if (triggerUpdate) {
setStep(1);
}
return <Text text="Bar" />;
}

const root = ReactNoop.createRoot();

await ReactNoop.act(async () => {
root.render(
<>
<Foo />
<Bar />
</>,
);
});
expect(Scheduler).toHaveYielded(['Foo [0]', 'Bar']);

// Bar will update Foo during its render phase. React should warn.
await ReactNoop.act(async () => {
root.render(
<>
<Foo />
<Bar triggerUpdate={true} />
</>,
);
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);
Expand Down

0 comments on commit ea6ed3d

Please sign in to comment.