Skip to content

Commit

Permalink
Don't log onRecoverableError if the current commit fail (#28665)
Browse files Browse the repository at this point in the history
We didn't recover after all.

Currently we might log a recoverable error in the recovery pass. E.g.
the SSR server had an error. Then the client component fails to render
which errors again. This ends up double logging.

So if we fail to actually complete a fully successful commit, we ignore
any recoverable errors because we'll get real errors logged.

It's possible that this might cover up some other error that happened at
the same time.
  • Loading branch information
sebmarkbage authored Mar 28, 2024
1 parent 323b6e9 commit e10a7b5
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 10 deletions.
18 changes: 9 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6004,23 +6004,23 @@ describe('ReactDOMFizzServer', () => {
expect(reportedServerErrors.length).toBe(1);
expect(reportedServerErrors[0].message).toBe('Oops!');

const reportedCaughtErrors = [];
const reportedClientErrors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onCaughtError(error) {
reportedCaughtErrors.push(error);
},
onRecoverableError(error) {
reportedClientErrors.push(error);
},
});
await waitForAll([]);
expect(getVisibleChildren(container)).toEqual('Oops!');
expect(reportedClientErrors.length).toBe(1);
if (__DEV__) {
expect(reportedClientErrors[0].message).toBe('Oops!');
} else {
expect(reportedClientErrors[0].message).toBe(
'The server could not finish this Suspense boundary, likely due to ' +
'an error during server rendering. Switched to client rendering.',
);
}
// Because this is rethrown on the client, it is not a recoverable error.
expect(reportedClientErrors.length).toBe(0);
// It is caught by the error boundary.
expect(reportedCaughtErrors.length).toBe(1);
expect(reportedCaughtErrors[0].message).toBe('Oops!');
});

it("use a promise that's already been instrumented and resolved", async () => {
Expand Down
9 changes: 8 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,14 @@ function finishConcurrentRender(
// Commit the placeholder.
break;
}
case RootErrored:
case RootErrored: {
// This render errored. Ignore any recoverable errors because we weren't actually
// able to recover. Instead, whatever the final errors were is the ones we log.
// This ensures that we only log the actual client side error if it's just a plain
// error thrown from a component on the server and the client.
workInProgressRootRecoverableErrors = null;
break;
}
case RootSuspended:
case RootCompleted: {
break;
Expand Down

0 comments on commit e10a7b5

Please sign in to comment.