Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn for update on different component in render #17099

Merged
merged 1 commit into from Feb 19, 2020
Merged

Warn for update on different component in render #17099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 33 additions & 19 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
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: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should include Block too?

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
View file
Open in desktop
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
44 changes: 44 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Open in desktop
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