Skip to content

Commit

Permalink
Don't retry synchronous render that doesn't go through Scheduler
Browse files Browse the repository at this point in the history
  • Loading branch information
@eps1lon
eps1lon committed Apr 22, 2024
1 parent 7bce2f9 commit 00d183b
Show file tree
Hide file tree
Showing 9 changed files with 136 additions and 155 deletions.
36 changes: 33 additions & 3 deletions packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js
View file
Expand Up @@ -752,7 +752,17 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll([
'A',
'B',
...(gate(flags => flags.enableUnifiedSyncLane)
? []
: [
// React will try one more time in non-blocking updates before giving up.
'A',
'B',
]),
]);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down Expand Up @@ -1684,7 +1694,17 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll([
'A',
'B',
...(gate(flags => flags.enableUnifiedSyncLane)
? []
: [
// React will try one more time in non-blocking updates before giving up.
'A',
'B',
]),
]);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down Expand Up @@ -2634,7 +2654,17 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll([
'A',
'B',
...(gate(flags => flags.enableUnifiedSyncLane)
? []
: [
// React will try one more time in non-blocking updates before giving up.
'A',
'B',
]),
]);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down
127 changes: 52 additions & 75 deletions packages/react-devtools-shared/src/__tests__/TimelineProfiler-test.js
View file
Expand Up @@ -2150,90 +2150,67 @@ describe('Timeline profiler', () => {
);

await waitForAll([
'ErrorBoundary render',
'ExampleThatThrows',
'ErrorBoundary render',
'ExampleThatThrows',
'ErrorBoundary fallback',
]);

const timelineData = stopProfilingAndGetTimelineData();
expect(timelineData.componentMeasures).toMatchInlineSnapshot(`
[
{
"componentName": "ErrorBoundary",
"duration": 10,
"timestamp": 10,
"type": "render",
"warning": null,
},
{
"componentName": "ExampleThatThrows",
"duration": 0,
"timestamp": 20,
"type": "render",
"warning": null,
},
{
"componentName": "ErrorBoundary",
"duration": 10,
"timestamp": 20,
"type": "render",
"warning": null,
},
{
"componentName": "ExampleThatThrows",
"duration": 0,
"timestamp": 30,
"type": "render",
"warning": null,
},
{
"componentName": "ErrorBoundary",
"duration": 10,
"timestamp": 30,
"type": "render",
"warning": null,
},
]
`);
[
{
"componentName": "ErrorBoundary",
"duration": 10,
"timestamp": 10,
"type": "render",
"warning": null,
},
{
"componentName": "ExampleThatThrows",
"duration": 0,
"timestamp": 20,
"type": "render",
"warning": null,
},
{
"componentName": "ErrorBoundary",
"duration": 10,
"timestamp": 20,
"type": "render",
"warning": null,
},
]
`);
expect(timelineData.schedulingEvents).toMatchInlineSnapshot(`
[
{
"lanes": "0b0000000000000000000000000100000",
"timestamp": 10,
"type": "schedule-render",
"warning": null,
},
{
"componentName": "ErrorBoundary",
"componentStack": "
in ErrorBoundary (at **)",
"lanes": "0b0000000000000000000000000000010",
"timestamp": 30,
"type": "schedule-state-update",
"warning": null,
},
]
`);
[
{
"lanes": "0b0000000000000000000000000100000",
"timestamp": 10,
"type": "schedule-render",
"warning": null,
},
{
"componentName": "ErrorBoundary",
"componentStack": "
in ErrorBoundary (at **)",
"lanes": "0b0000000000000000000000000000010",
"timestamp": 20,
"type": "schedule-state-update",
"warning": null,
},
]
`);
expect(timelineData.thrownErrors).toMatchInlineSnapshot(`
[
{
"componentName": "ExampleThatThrows",
"message": "Expected error",
"phase": "mount",
"timestamp": 20,
"type": "thrown-error",
},
{
"componentName": "ExampleThatThrows",
"message": "Expected error",
"phase": "mount",
"timestamp": 30,
"type": "thrown-error",
},
]
`);
[
{
"componentName": "ExampleThatThrows",
"message": "Expected error",
"phase": "mount",
"timestamp": 20,
"type": "thrown-error",
},
]
`);
});

it('should mark passive and layout effects', async () => {
Expand Down
9 changes: 4 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Expand Up @@ -1364,14 +1364,13 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
}

let exitStatus = renderRootSync(root, lanes);
const wasRootDehydrated = supportsHydration && isRootDehydrated(root);
if (
(disableLegacyMode || root.tag !== LegacyRoot) &&
exitStatus === RootErrored
exitStatus === RootErrored &&
wasRootDehydrated
) {
// If something threw an error, try rendering one more time. We'll render
// synchronously to block concurrent data mutations, and we'll includes
// all pending updates are included. If it still fails after the second
// attempt, we'll give up and commit the resulting tree.
// If something threw an error and we have work to do, try rendering one more time.
const originallyAttemptedLanes = lanes;
const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
root,
Expand Down
81 changes: 37 additions & 44 deletions packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View file
Expand Up @@ -380,7 +380,18 @@ describe('ReactIncrementalErrorHandling', () => {
// Render the bad component synchronously
ReactNoop.render(<Parent />, () => Scheduler.log('commit'));

await waitFor(['Parent', 'BadRender', 'commit']);
await waitFor([
'Parent',
'BadRender',
...(gate(flags => flags.enableUnifiedSyncLane)
? []
: [
// Retry in non-blocking updates
'Parent',
'BadRender',
]),
'commit',
]);
expect(ReactNoop).toMatchRenderedOutput(null);
});

Expand Down Expand Up @@ -410,7 +421,7 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop.render(<Parent />, () => Scheduler.log('commit'));
});

assertLog(['Parent', 'BadRender', 'Parent', 'BadRender', 'commit']);
assertLog(['Parent', 'BadRender', 'commit']);
expect(ReactNoop).toMatchRenderedOutput(null);
});

Expand Down Expand Up @@ -666,12 +677,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'ErrorBoundary render success',
'BrokenRender',

// React retries one more time
'ErrorBoundary render success',
'BrokenRender',

// Errored again on retry. Now handle it.
'ErrorBoundary componentDidCatch',
'ErrorBoundary render error',
]);
Expand Down Expand Up @@ -716,12 +721,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'ErrorBoundary render success',
'BrokenRender',

// React retries one more time
'ErrorBoundary render success',
'BrokenRender',

// Errored again on retry. Now handle it.
'ErrorBoundary componentDidCatch',
'ErrorBoundary render error',
]);
Expand Down Expand Up @@ -840,12 +839,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'RethrowErrorBoundary render',
'BrokenRender',

// React retries one more time
'RethrowErrorBoundary render',
'BrokenRender',

// Errored again on retry. Now handle it.
'RethrowErrorBoundary componentDidCatch',
]);
expect(ReactNoop).toMatchRenderedOutput(null);
Expand Down Expand Up @@ -883,10 +876,6 @@ describe('ReactIncrementalErrorHandling', () => {
'RethrowErrorBoundary render',
'BrokenRender',

// React retries one more time
'RethrowErrorBoundary render',
'BrokenRender',

// Errored again on retry. Now handle it.
'RethrowErrorBoundary componentDidCatch',
]);
Expand Down Expand Up @@ -1892,27 +1881,31 @@ describe('ReactIncrementalErrorHandling', () => {
root.render(<Oops />);
});

await 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.']);
await expect(
act(async () => {
// Schedule a default pri and a low pri update on the root.
root.render(<Oops />);
React.startTransition(() => {
root.render(<AllGood />);
});

// 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.');
// 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.
// 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.
// 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);
});
}),
).rejects.toThrow('Oops');
assertLog([]);
expect(root).toMatchRenderedOutput(null);
});

it("does not infinite loop if there's a render phase update in the same render as an error", async () => {
Expand Down
13 changes: 1 addition & 12 deletions packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js
View file
Expand Up @@ -249,18 +249,7 @@ describe('ReactIncrementalErrorLogging', () => {
<Foo />
</ErrorBoundary>,
);
await waitForAll(
[
'render: 0',

'render: 1',

// Retry one more time before handling error
'render: 1',

'componentWillUnmount: 0',
].filter(Boolean),
);
await waitForAll(['render: 0', 'render: 1', 'componentWillUnmount: 0']);

expect(console.error).toHaveBeenCalledTimes(1);
if (__DEV__) {
Expand Down
6 changes: 1 addition & 5 deletions packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
View file
Expand Up @@ -56,11 +56,7 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
ReactDOM.flushSync ->
root.render React.createElement(Foo)
).toErrorDev([
# A failed component renders twice in DEV in concurrent mode
'No `render` method found on the Foo instance',
'No `render` method found on the Foo instance',
])
).toErrorDev(['No `render` method found on the Foo instance'])
window.removeEventListener 'error', errorHandler;
expect(caughtErrors).toEqual([
expect.objectContaining(
Expand Down
3 changes: 0 additions & 3 deletions packages/react/src/__tests__/ReactES6Class-test.js
View file
Expand Up @@ -70,9 +70,6 @@ describe('ReactES6Class', () => {
expect(() => {
ReactDOM.flushSync(() => root.render(<Foo />));
}).toErrorDev([
// A failed component renders twice in DEV in concurrent mode
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
]);
Expand Down

0 comments on commit 00d183b

Please sign in to comment.