From bd027064f50adad3b2d6e077fbf4238ad7cbaeb5 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 20 May 2024 23:53:52 -0400 Subject: [PATCH] Set the current fiber to the source of the error during error reporting 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. # Conflicts: # packages/shared/consoleWithStackDev.js --- .../src/__tests__/treeContext-test.js | 22 ++--- .../ReactDOMConsoleErrorReporting-test.js | 12 +-- ...eactDOMConsoleErrorReportingLegacy-test.js | 16 ++-- .../ReactErrorBoundaries-test.internal.js | 2 +- ...eactLegacyErrorBoundaries-test.internal.js | 2 +- .../src/ReactFiberErrorLogger.js | 88 +++++++++++++------ .../react-reconciler/src/ReactFiberThrow.js | 13 +++ .../src/ReactFiberWorkLoop.js | 2 + ...tIncrementalErrorHandling-test.internal.js | 2 +- .../ReactIncrementalErrorLogging-test.js | 46 +++++----- packages/shared/consoleWithStackDev.js | 22 +++-- scripts/eslint-rules/warning-args.js | 7 +- 12 files changed, 148 insertions(+), 86 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 185e11461bfe4..bbe9d9a17acb6 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2586,16 +2586,14 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 1, ⚠ 0 [root] - ✕ + `); selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 1, ⚠ 0 [root] - → ✕ + `); utils.act(() => unmount()); @@ -2648,16 +2646,14 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 1, ⚠ 0 [root] - ✕ + `); selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 1, ⚠ 0 [root] - → ✕ + `); utils.act(() => unmount()); @@ -2705,18 +2701,18 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] - ▾ ✕ + ▾ ✕ `); selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] - → ▾ ✕ - ✕ + ▾ + → ✕ `); }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index 65aa67d342579..9eeb4e7c072a1 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js @@ -142,8 +142,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), - expect.stringContaining('Foo'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -207,8 +207,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -273,8 +273,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), - expect.stringContaining('Foo'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -343,8 +343,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -409,8 +409,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), - expect.stringContaining('Foo'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -477,8 +477,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js index 02e34f9322f10..65301c789d0f0 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js @@ -161,8 +161,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), // Addendum by React: expect.stringContaining('An error occurred in the component'), - expect.stringContaining('Foo'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); @@ -238,8 +238,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -307,11 +307,9 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), // Addendum by React: - expect.stringContaining( - 'An error occurred in the component:', - ), - expect.stringContaining('Foo'), + expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); @@ -391,8 +389,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { @@ -461,8 +459,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), - expect.stringContaining('Foo'), expect.stringContaining('Consider adding an error boundary'), + expect.stringContaining('Foo'), ], ]); @@ -541,8 +539,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining( 'The above error occurred in the component', ), - expect.stringContaining('Foo'), expect.stringContaining('ErrorBoundary'), + expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 684f011cb6009..f45069e91539c 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -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 component:', + 'The above error occurred in the component', ); } diff --git a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js index b0b223dd43bee..2167d819cbbb8 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js @@ -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 component:', + 'The above error occurred in the component', ); } diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index 381346e6b4a95..a948e5c79b694 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -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; @@ -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, + ); + } } } @@ -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. } } diff --git a/packages/react-reconciler/src/ReactFiberThrow.js b/packages/react-reconciler/src/ReactFiberThrow.js index 8393e3d007a23..40a68b1c047b0 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.js +++ b/packages/react-reconciler/src/ReactFiberThrow.js @@ -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, @@ -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; } @@ -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); }; } @@ -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. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index e46927c1bc498..9c0a3d3f15c07 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -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(); } } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index c6e342871b198..b0ac81016e93b 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -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 component:', + 'The above error occurred in the component', ); } else { expect(console.error).toHaveBeenCalledTimes(1); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index d4c7949580c97..3bd934af38da6 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -85,7 +85,11 @@ describe('ReactIncrementalErrorLogging', () => { expect(console.warn).toHaveBeenCalledWith( expect.stringContaining('%s'), expect.stringContaining( - 'An error occurred in the component:', + 'An error occurred in the component.', + ), + expect.stringContaining( + 'Consider adding an error boundary to your tree ' + + 'to customize error handling behavior.', ), expect.stringMatching( new RegExp( @@ -94,10 +98,6 @@ describe('ReactIncrementalErrorLogging', () => { '\\s+(in|at) div(.*)', ), ), - expect.stringContaining( - 'Consider adding an error boundary to your tree ' + - 'to customize error handling behavior.', - ), ); } }); @@ -131,7 +131,11 @@ describe('ReactIncrementalErrorLogging', () => { expect(console.warn).toHaveBeenCalledWith( expect.stringContaining('%s'), expect.stringContaining( - 'An error occurred in the component:', + 'An error occurred in the component.', + ), + expect.stringContaining( + 'Consider adding an error boundary to your tree ' + + 'to customize error handling behavior.', ), expect.stringMatching( new RegExp( @@ -140,10 +144,6 @@ describe('ReactIncrementalErrorLogging', () => { '\\s+(in|at) div(.*)', ), ), - expect.stringContaining( - 'Consider adding an error boundary to your tree ' + - 'to customize error handling behavior.', - ), ); } }); @@ -189,7 +189,11 @@ describe('ReactIncrementalErrorLogging', () => { message: 'render error', }), expect.stringContaining( - 'The above error occurred in the component:', + 'The above error occurred in the component.', + ), + expect.stringContaining( + 'React will try to recreate this component tree from scratch ' + + 'using the error boundary you provided, ErrorBoundary.', ), expect.stringMatching( new RegExp( @@ -199,10 +203,6 @@ describe('ReactIncrementalErrorLogging', () => { '\\s+(in|at) div(.*)', ), ), - expect.stringContaining( - 'React will try to recreate this component tree from scratch ' + - 'using the error boundary you provided, ErrorBoundary.', - ), ); } else { expect(logCapturedErrorCalls[0]).toEqual( @@ -270,17 +270,21 @@ describe('ReactIncrementalErrorLogging', () => { message: 'oops', }), expect.stringContaining( - 'The above error occurred in the component:', - ), - expect.stringMatching( - new RegExp( - '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', - ), + 'The above error occurred in the component.', ), expect.stringContaining( 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundary.', ), + expect.stringMatching( + gate(flag => flag.enableOwnerStacks) + ? // With owner stacks the return path is cut off but in this case + // this is also what the owner stack looks like. + new RegExp('\\s+(in|at) Foo (.*)') + : new RegExp( + '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', + ), + ), ); } else { expect(console.error).toHaveBeenCalledWith( diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js index d815d8796d809..1061aaf1821c1 100644 --- a/packages/shared/consoleWithStackDev.js +++ b/packages/shared/consoleWithStackDev.js @@ -6,6 +6,7 @@ */ import ReactSharedInternals from 'shared/ReactSharedInternals'; +import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; let suppressWarning = false; export function setSuppressWarning(newSuppressWarning) { @@ -40,19 +41,30 @@ function printWarning(level, format, args) { // When changing this logic, you might want to also // update consoleWithStackDev.www.js as well. if (__DEV__) { + const isErrorLogger = + format === '%s\n\n%s\n' || format === '%o\n\n%s\n\n%s\n'; + const stack = ReactSharedInternals.getStackAddendum(); if (stack !== '') { format += '%s'; args = args.concat([stack]); } - // eslint-disable-next-line react-internal/safe-string-coercion - const argsWithFormat = args.map(item => String(item)); - // Careful: RN currently depends on this prefix - argsWithFormat.unshift('Warning: ' + format); + if (isErrorLogger) { + // Don't prefix our default logging formatting in ReactFiberErrorLoggger. + // Don't toString the arguments. + args.unshift(format); + } else { + // TODO: Remove this prefix and stop toStringing in the wrapper and + // instead do it at each callsite as needed. + // Careful: RN currently depends on this prefix + // eslint-disable-next-line react-internal/safe-string-coercion + args = args.map(item => String(item)); + args.unshift('Warning: ' + format); + } // We intentionally don't use spread (or .apply) directly because it // breaks IE9: https://github.com/facebook/react/issues/13610 // eslint-disable-next-line react-internal/no-production-logging - Function.prototype.apply.call(console[level], console, argsWithFormat); + Function.prototype.apply.call(console[level], console, args); } } diff --git a/scripts/eslint-rules/warning-args.js b/scripts/eslint-rules/warning-args.js index 701d4db267915..47df4e519a9c5 100644 --- a/scripts/eslint-rules/warning-args.js +++ b/scripts/eslint-rules/warning-args.js @@ -73,7 +73,10 @@ module.exports = { ); return; } - if (format.length < 10 || /^[s\W]*$/.test(format)) { + if ( + (format.length < 10 || /^[s\W]*$/.test(format)) && + format !== '%s\n\n%s\n' + ) { context.report( node, 'The {{name}} format should be able to uniquely identify this ' + @@ -83,7 +86,7 @@ module.exports = { return; } // count the number of formatting substitutions, plus the first two args - const expectedNArgs = (format.match(/%s/g) || []).length + 1; + const expectedNArgs = (format.match(/%[so]/g) || []).length + 1; if (node.arguments.length !== expectedNArgs) { context.report( node,