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

Set the current fiber to the source of the error during error reporting #29044

Merged
merged 1 commit into from
May 23, 2024
Merged

Set the current fiber to the source of the error during error reporting #29044

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
22 changes: 9 additions & 13 deletions packages/react-devtools-shared/src/__tests__/treeContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,16 +2586,14 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));

expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
<ErrorBoundary> ✕
<ErrorBoundary>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What happens here is that now the console.error happens in the context of the source of the error, not in the error boundary, but that node gets deleted so it's no longer part of the tree.

That's what happens to the console.error already which is why it was only one marked here and not two (one for console.error and one for throw). (It was one before this PR and two before the stack but the other one is representing the console.error in TestRenderer, so it was the wrong one.)

It seems like maybe this UI should be adjusted but we have similar problems with infinitely suspended components whose fiber never commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like maybe this UI should be adjusted but we have similar problems with infinitely suspended components whose fiber never commits.

Ideally you'd still be able to narrow down the console.error. It was likely confusing to attach the error to the error boundary when that's not where it originated. But some indicator that the error originated from somewhere within this tree would be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it was attached to the error boundary but dropped before too. The thrown was attached to the error boundary.

The other previous error came from the TestRenderer warning about using test renderer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logs are tricky because there can be a done of completely unrelated logs in a subtree like the whole page that unmount. Not necessarily associated with the error that triggered the error boundary.

However, the actual error not being marked is clearly an issue.

`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
→ <ErrorBoundary> ✕
<ErrorBoundary>
`);

utils.act(() => unmount());
Expand Down Expand Up @@ -2648,16 +2646,14 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));

expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
<ErrorBoundary> ✕
<ErrorBoundary>
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
→ <ErrorBoundary> ✕
<ErrorBoundary>
`);

utils.act(() => unmount());
Expand Down Expand Up @@ -2705,18 +2701,18 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));

expect(store).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
▾ <ErrorBoundary> ✕
▾ <ErrorBoundary>
<Child> ✕
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
→ ▾ <ErrorBoundary> ✕
<Child> ✕
▾ <ErrorBoundary>
→ <Child> ✕
`);
});
});
Expand Down
12 changes: 6 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
// Addendum by React:
expect.stringContaining('%s'),
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Foo'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -207,8 +207,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -273,8 +273,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
// Addendum by React:
expect.stringContaining('%s'),
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Foo'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -343,8 +343,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -409,8 +409,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
// Addendum by React:
expect.stringContaining('%s'),
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Foo'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -477,8 +477,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down
16 changes: 7 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining('%s'),
// Addendum by React:
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Foo'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);

Expand Down Expand Up @@ -238,8 +238,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -307,11 +307,9 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining('%s'),

// Addendum by React:
expect.stringContaining(
'An error occurred in the <Foo> component:',
),
expect.stringContaining('Foo'),
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);

Expand Down Expand Up @@ -391,8 +389,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down Expand Up @@ -461,8 +459,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

// Addendum by React:
expect.stringContaining('An error occurred in the <Foo> component'),
expect.stringContaining('Foo'),
expect.stringContaining('Consider adding an error boundary'),
expect.stringContaining('Foo'),
],
]);

Expand Down Expand Up @@ -541,8 +539,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
expect.stringContaining('Foo'),
expect.stringContaining('ErrorBoundary'),
expect.stringContaining('Foo'),
],
]);
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ describe('ReactErrorBoundaries', () => {
if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.mock.calls[0][2]).toContain(
'The above error occurred in the <BrokenRender> component:',
'The above error occurred in the <BrokenRender> component',
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ describe('ReactLegacyErrorBoundaries', () => {
'ReactDOM.render has not been supported since React 18',
);
expect(console.error.mock.calls[1][2]).toContain(
'The above error occurred in the <BrokenRender> component:',
'The above error occurred in the <BrokenRender> component',
);
}

Expand Down
88 changes: 61 additions & 27 deletions packages/react-reconciler/src/ReactFiberErrorLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import reportGlobalError from 'shared/reportGlobalError';

import ReactSharedInternals from 'shared/ReactSharedInternals';

import {enableOwnerStacks} from 'shared/ReactFeatureFlags';

// Side-channel since I'm not sure we want to make this part of the public API
let componentName: null | string = null;
let errorBoundaryName: null | string = null;
Expand All @@ -34,20 +36,36 @@ export function defaultOnUncaughtError(
// So we add those into a separate console.warn.
reportGlobalError(error);
if (__DEV__) {
const componentStack =
errorInfo.componentStack != null ? errorInfo.componentStack : '';

const componentNameMessage = componentName
? `An error occurred in the <${componentName}> component:`
: 'An error occurred in one of your React components:';
? `An error occurred in the <${componentName}> component.`
: 'An error occurred in one of your React components.';

console['warn'](
'%s\n%s\n\n%s',
componentNameMessage,
componentStack || '',
const errorBoundaryMessage =
'Consider adding an error boundary to your tree to customize error handling behavior.\n' +
'Visit https://react.dev/link/error-boundaries to learn more about error boundaries.',
);
'Visit https://react.dev/link/error-boundaries to learn more about error boundaries.';

if (enableOwnerStacks) {
console.warn(
'%s\n\n%s\n',
componentNameMessage,
errorBoundaryMessage,
// We let our console.error wrapper add the component stack to the end.
);
} else {
// The current Fiber is disconnected at this point which means that console printing
// cannot add a component stack since it terminates at the deletion node. This is not
// a problem for owner stacks which are not disconnected but for the parent component
// stacks we need to use the snapshot we've previously extracted.
const componentStack =
errorInfo.componentStack != null ? errorInfo.componentStack : '';
// Don't transform to our wrapper
console['warn'](
'%s\n\n%s\n%s',
componentNameMessage,
errorBoundaryMessage,
componentStack,
);
}
}
}

Expand All @@ -63,31 +81,47 @@ export function defaultOnCaughtError(

// Caught by error boundary
if (__DEV__) {
const componentStack =
errorInfo.componentStack != null ? errorInfo.componentStack : '';

const componentNameMessage = componentName
? `The above error occurred in the <${componentName}> component:`
: 'The above error occurred in one of your React components:';
? `The above error occurred in the <${componentName}> component.`
: 'The above error occurred in one of your React components.';

// In development, we provide our own message which includes the component stack
// in addition to the error.
// Don't transform to our wrapper
console['error'](
'%o\n\n%s\n%s\n\n%s',
error,
componentNameMessage,
componentStack,
const recreateMessage =
`React will try to recreate this component tree from scratch ` +
`using the error boundary you provided, ${
errorBoundaryName || 'Anonymous'
}.`,
);
`using the error boundary you provided, ${
errorBoundaryName || 'Anonymous'
}.`;

if (enableOwnerStacks) {
console.error(
'%o\n\n%s\n\n%s\n',
error,
componentNameMessage,
recreateMessage,
// We let our consoleWithStackDev wrapper add the component stack to the end.
);
} else {
// The current Fiber is disconnected at this point which means that console printing
// cannot add a component stack since it terminates at the deletion node. This is not
// a problem for owner stacks which are not disconnected but for the parent component
// stacks we need to use the snapshot we've previously extracted.
const componentStack =
errorInfo.componentStack != null ? errorInfo.componentStack : '';
// Don't transform to our wrapper
console['error'](
'%o\n\n%s\n\n%s\n%s',
error,
componentNameMessage,
recreateMessage,
componentStack,
);
}
} else {
// In production, we print the error directly.
// This will include the message, the JS stack, and anything the browser wants to show.
// We pass the error object instead of custom message so that the browser displays the error natively.
console['error'](error); // Don't transform to our wrapper
console['error'](error); // Don't transform to our wrapper, however, React DevTools can still add a stack.
}
}

Expand Down
13 changes: 13 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ import {
import {ConcurrentRoot} from './ReactRootTags';
import {noopSuspenseyCommitThenable} from './ReactFiberThenable';
import {REACT_POSTPONE_TYPE} from 'shared/ReactSymbols';
import {
setCurrentDebugFiberInDEV,
getCurrentFiber as getCurrentDebugFiberInDEV,
} from './ReactCurrentFiber';

function createRootErrorUpdate(
root: FiberRoot,
Expand All @@ -100,7 +104,10 @@ function createRootErrorUpdate(
// being called "element".
update.payload = {element: null};
update.callback = () => {
const prevFiber = getCurrentDebugFiberInDEV(); // should just be the root
setCurrentDebugFiberInDEV(errorInfo.source);
logUncaughtError(root, errorInfo);
setCurrentDebugFiberInDEV(prevFiber);
};
return update;
}
Expand All @@ -127,7 +134,10 @@ function initializeClassErrorUpdate(
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
const prevFiber = getCurrentDebugFiberInDEV(); // should be the error boundary
setCurrentDebugFiberInDEV(errorInfo.source);
logCaughtError(root, fiber, errorInfo);
setCurrentDebugFiberInDEV(prevFiber);
};
}

Expand All @@ -138,7 +148,10 @@ function initializeClassErrorUpdate(
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
const prevFiber = getCurrentDebugFiberInDEV(); // should be the error boundary
setCurrentDebugFiberInDEV(errorInfo.source);
logCaughtError(root, fiber, errorInfo);
setCurrentDebugFiberInDEV(prevFiber);
if (typeof getDerivedStateFromError !== 'function') {
// To preserve the preexisting retry behavior of error boundaries,
// we keep track of which ones already failed during this batch.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3038,7 +3038,9 @@ function commitRootImpl(
for (let i = 0; i < recoverableErrors.length; i++) {
const recoverableError = recoverableErrors[i];
const errorInfo = makeErrorInfo(recoverableError.stack);
setCurrentDebugFiberInDEV(recoverableError.source);
onRecoverableError(recoverableError.value, errorInfo);
resetCurrentDebugFiberInDEV();
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,7 +1512,7 @@ describe('ReactIncrementalErrorHandling', () => {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.mock.calls[0][1]).toBe(notAnError);
expect(console.error.mock.calls[0][2]).toContain(
'The above error occurred in the <BadRender> component:',
'The above error occurred in the <BadRender> component',
);
} else {
expect(console.error).toHaveBeenCalledTimes(1);
Expand Down