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.
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
Reconciler: Don't retry synchronous render #28810
base: main
Are you sure you want to change the base?
Conversation
@@ -1066,10 +1066,10 @@ describe(`onRender`, () => { | |||
|
|||
// start time: 5 initially + 14 of work | |||
// Add an additional 3 (ThrowsError) if we replayed the failed work | |||
expect(mountCall[4]).toBe(19); | |||
expect(mountCall[4]).toBe(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Update comments
Comparing: 5b903cd...00d183b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
f5663e5
to
ced79ba
Compare
// Check if something threw | ||
if (exitStatus === RootErrored) { | ||
// Check if something threw. | ||
// We only need to retry during hydration or concurrent render. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't even do it for those because we should refactor the Root recovery to work the same way as Suspense recovery.
ced79ba
to
cd59515
Compare
await act(async () => { | ||
// Schedule a default pri and a low pri update on the root. | ||
root.render(<Oops />); | ||
React.startTransition(() => { | ||
root.render(<AllGood />); | ||
}); | ||
await expect( | ||
act(async () => { | ||
// Schedule a default pri and a low pri update on the root. | ||
root.render(<Oops />); | ||
React.startTransition(() => { | ||
root.render(<AllGood />); | ||
}); | ||
|
||
// Render through just the default pri update. The low pri update remains on | ||
// the queue. | ||
await waitFor(['Everything is fine.']); | ||
// Render through the default pri and low pri update. | ||
await waitFor(['Everything is fine.']); | ||
|
||
// Schedule a discrete update on a child that triggers an error. | ||
// The root should capture this error. But since there's still a pending | ||
// update on the root, the error should be suppressed. | ||
ReactNoop.discreteUpdates(() => { | ||
setShouldThrow(true); | ||
}); | ||
}); | ||
// Should render the final state without throwing the error. | ||
assertLog(['Everything is fine.']); | ||
expect(root).toMatchRenderedOutput('Everything is fine.'); | ||
// Schedule a discrete update on a child that triggers an error. | ||
ReactNoop.discreteUpdates(() => { | ||
setShouldThrow(true); | ||
}); | ||
}), | ||
).rejects.toThrow('Oops'); | ||
assertLog([]); | ||
expect(root).toMatchRenderedOutput(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs discussion. Might be fine since we already rendered the low-pri update before we error. So last render wins?
b3a570c
to
2736d41
Compare
2736d41
to
00d183b
Compare
expect(root).toMatchRenderedOutput('Count 0 (n=1)'); | ||
expect(Text).toBeCalledTimes(2); | ||
if (gate(flags => flags.enableUnifiedSyncLane)) { | ||
expect(root).toMatchRenderedOutput(<h1>Something went wrong.</h1>); | ||
expect(Text).toBeCalledTimes(1); | ||
} else { | ||
expect(root).toMatchRenderedOutput('Count 0 (n=1)'); | ||
expect(Text).toBeCalledTimes(2); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions implicitly tested the error recovery. In concurrent updates we'd retry and this component only throws once:
if (shouldFail) {
shouldFail = false;
throw new Error('failed');
}
Summary
In #13041 we started retrying the render synchronously if a render threw during a concurrent render. However, this also applied to synchronous renders which should help very rarely since we don't support mutations during render. The original PR even explicitly said that a synchronous render should not be retried. We also didn't used to retry in legacy roots: https://react-error-recovery-showcase.vercel.app/
A lot of tests asserted on the retry even though their render was sync i.e. not wrapped in
startTransition
.I originally caught this while investigating duplicate host instance creation in error boundary fallbacks. But this is expected by proxy of having error recovery for sync renders. Host instances are created during render phase.
How did you test this change?
[ ] Codesandbox in Bug: Elements in ErrorBoundary are created twice, one of them becomes an orphan #26518 no longer callsWe also create host instances twice when recovering from an error during concurrent render. We just never mutate the DOM twice. Creating host instances during render is intentional and documenteddocument.createElement
twice