Skip to content

Commit

Permalink
Set the current fiber to the source of the error during error reporting
Browse files Browse the repository at this point in the history
This lets us expose the component stack to the error reporting that happens
here as console.error patching, but more importantly for "owner stacks"
this will be able to set the native async stacks to the original fiber.

We now use the normal console.error management to add a component stack to
the end.
  • Loading branch information
sebmarkbage committed May 10, 2024
1 parent 0812517 commit 9e54e8f
Show file tree
Hide file tree
Showing 12 changed files with 144 additions and 86 deletions.
22 changes: 9 additions & 13 deletions packages/react-devtools-shared/src/__tests__/treeContext-test.js
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>
`);

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
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
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -3029,7 +3029,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
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
Loading

0 comments on commit 9e54e8f

Please sign in to comment.