From aae244d7aba332bb3272afac889fdf8607613407 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sat, 9 Mar 2024 19:08:08 -0800 Subject: [PATCH] Print the error in DEV with addendum Since we no longer get the error printed by the browser automatically we have to manually print the error with an addendum which contains component stacks. --- .../ReactDOMConsoleErrorReporting-test.js | 42 +++++ ...eactDOMConsoleErrorReportingLegacy-test.js | 47 ++++- .../ReactErrorBoundaries-test.internal.js | 2 +- ...eactLegacyErrorBoundaries-test.internal.js | 2 +- .../src/ReactFiberCommitWork.js | 15 -- .../src/ReactFiberErrorLogger.js | 41 ++--- .../src/ReactFiberWorkLoop.js | 2 - ...tIncrementalErrorHandling-test.internal.js | 5 +- .../ReactIncrementalErrorLogging-test.js | 173 +++++++++++------- .../useSyncExternalStoreShared-test.js | 1 + scripts/jest/shouldIgnoreConsoleError.js | 11 +- 11 files changed, 219 insertions(+), 122 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index b001fcf3dcacc..bd80905478bf0 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js @@ -130,10 +130,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -183,10 +190,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { @@ -236,10 +250,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -292,10 +313,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { @@ -345,10 +373,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -401,10 +436,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js index c40dff4e938fd..e75b4b5e29045 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js @@ -159,10 +159,17 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -215,12 +222,18 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], - // TODO: There is no log for this error now. [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { @@ -271,12 +284,18 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], - // TODO: There should be an error reported here. [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -332,12 +351,18 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], - // TODO: There should be a log here. [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { @@ -389,12 +414,18 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], - // TODO: We should have logged an error. [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('Consider adding an error boundary'), ], ]); } else { @@ -450,12 +481,18 @@ describe('ReactDOMConsoleErrorReporting', () => { expect(windowOnError.mock.calls).toEqual([]); expect(console.error.mock.calls).toEqual([ [expect.stringContaining('ReactDOM.render is no longer supported')], - // TODO: There should be a log here. [ + // Formatting + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'Boom', + }), // Addendum by React: expect.stringContaining( 'The above error occurred in the component', ), + expect.stringContaining('Foo'), + expect.stringContaining('ErrorBoundary'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 4170168ddc822..284cb0599aded 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -707,7 +707,7 @@ describe('ReactErrorBoundaries', () => { }); if (__DEV__) { expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.mock.calls[0][0]).toContain( + expect(console.error.mock.calls[0][2]).toContain( '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 960ba074c1ea1..bed800fb1da73 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js @@ -686,7 +686,7 @@ describe('ReactLegacyErrorBoundaries', () => { expect(console.error.mock.calls[0][0]).toContain( 'ReactDOM.render is no longer supported', ); - expect(console.error.mock.calls[1][0]).toContain( + expect(console.error.mock.calls[1][2]).toContain( 'The above error occurred in the component:', ); } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 8ee7165e3f645..fdd289d93280f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -243,21 +243,6 @@ function shouldProfile(current: Fiber): boolean { ); } -export function reportUncaughtErrorInDEV(error: mixed) { - // Wrapping each small part of the commit phase into a guarded - // callback is a bit too slow (https://github.com/facebook/react/pull/21666). - // But we rely on it to surface errors to DEV tools like overlays - // (https://github.com/facebook/react/issues/21712). - // As a compromise, rethrow only caught errors in a guard. - if (__DEV__) { - // TODO: This trick no longer works. Should probably use reportError maybe. - // invokeGuardedCallback(null, () => { - // throw error; - // }); - // clearCaughtError(); - } -} - function callComponentWillUnmountWithTimer(current: Fiber, instance: any) { instance.props = current.memoizedProps; instance.state = current.memoizedState; diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index 7eaff9a716923..14a1ebb352f89 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -11,7 +11,6 @@ import type {Fiber} from './ReactInternalTypes'; import type {CapturedValue} from './ReactCapturedValue'; import {showErrorDialog} from './ReactFiberErrorDialog'; -import {ClassComponent} from './ReactWorkTags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import {HostRoot} from 'react-reconciler/src/ReactWorkTags'; @@ -33,25 +32,8 @@ export function logCapturedError( const source = errorInfo.source; const stack = errorInfo.stack; const componentStack = stack !== null ? stack : ''; - // Browsers support silencing uncaught errors by calling - // `preventDefault()` in window `error` handler. - // We record this information as an expando on the error. - // TODO: We no longer track this. - if (error != null && error._suppressLogging) { - if (boundary.tag === ClassComponent) { - // The error is recoverable and was silenced. - // Ignore it and don't print the stack addendum. - // This is handy for testing error boundaries without noise. - return; - } - // The error is fatal. Since the silencing might have - // been accidental, we'll surface it anyway. - // However, the browser would have silenced the original error - // so we'll print it first, and then print the stack addendum. - console['error'](error); // Don't transform to our wrapper - // For a more detailed description of this block, see: - // https://github.com/facebook/react/pull/13384 - } + // TODO: There's no longer a way to silence these warnings e.g. for tests. + // See https://github.com/facebook/react/pull/13384 const componentName = source ? getComponentNameFromFiber(source) : null; const componentNameMessage = componentName @@ -70,16 +52,17 @@ export function logCapturedError( `React will try to recreate this component tree from scratch ` + `using the error boundary you provided, ${errorBoundaryName}.`; } - const combinedMessage = - `${componentNameMessage}\n${componentStack}\n\n` + - `${errorBoundaryMessage}`; - // TODO: The error is no longer printed by the browser. - // In development, we provide our own message with just the component stack. - // We don't include the original error message and JS stack because the browser - // has already printed it. Even if the application swallows the error, it is still - // displayed by the browser thanks to the DEV-only fake event trick in ReactErrorUtils. - console['error'](combinedMessage); // Don't transform to our wrapper + // In development, we provide our own message which includes the component stack + // in addition to the error. + console['error']( + // Don't transform to our wrapper + '%o\n\n%s\n%s\n\n%s', + error, + componentNameMessage, + componentStack, + errorBoundaryMessage, + ); } else { // In production, we print the error directly. // This will include the message, the JS stack, and anything the browser wants to show. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 1693c617a78eb..152b0b460b2f4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -186,7 +186,6 @@ import { reconnectPassiveEffects, reappearLayoutEffects, disconnectPassiveEffect, - reportUncaughtErrorInDEV, invokeLayoutEffectMountInDEV, invokePassiveEffectMountInDEV, invokeLayoutEffectUnmountInDEV, @@ -3384,7 +3383,6 @@ export function captureCommitPhaseError( error: mixed, ) { if (__DEV__) { - reportUncaughtErrorInDEV(error); setIsRunningInsertionEffect(false); } if (sourceFiber.tag === HostRoot) { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index f519b0710ffa0..1613f1e048ec7 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1510,7 +1510,8 @@ describe('ReactIncrementalErrorHandling', () => { if (__DEV__) { expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.mock.calls[0][0]).toContain( + expect(console.error.mock.calls[0][1]).toBe(notAnError); + expect(console.error.mock.calls[0][2]).toContain( 'The above error occurred in the component:', ); } else { @@ -1911,7 +1912,7 @@ describe('ReactIncrementalErrorHandling', () => { expect(console.error.mock.calls[0][0]).toContain( 'Cannot update a component (`%s`) while rendering a different component', ); - expect(console.error.mock.calls[1][0]).toContain( + expect(console.error.mock.calls[1][2]).toContain( 'The above error occurred in the component', ); } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index adfeb238695dd..82edcf2db4503 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -60,22 +60,34 @@ describe('ReactIncrementalErrorLogging', () => { ); await waitForThrow('constructor error'); expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error).toHaveBeenCalledWith( - __DEV__ - ? expect.stringMatching( - new RegExp( - 'The above error occurred in the component:\n' + - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)\n\n' + - 'Consider adding an error boundary to your tree ' + - 'to customize error handling behavior\\.', - ), - ) - : expect.objectContaining({ - message: 'constructor error', - }), - ); + if (__DEV__) { + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'constructor error', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), + expect.stringMatching( + new RegExp( + '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) div(.*)', + ), + ), + expect.stringContaining( + 'Consider adding an error boundary to your tree ' + + 'to customize error handling behavior.', + ), + ); + } else { + expect(console.error).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'constructor error', + }), + ); + } }); it('should log errors that occur during the commit phase', async () => { @@ -96,22 +108,34 @@ describe('ReactIncrementalErrorLogging', () => { ); await waitForThrow('componentDidMount error'); expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error).toHaveBeenCalledWith( - __DEV__ - ? expect.stringMatching( - new RegExp( - 'The above error occurred in the component:\n' + - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)\n\n' + - 'Consider adding an error boundary to your tree ' + - 'to customize error handling behavior\\.', - ), - ) - : expect.objectContaining({ - message: 'componentDidMount error', - }), - ); + if (__DEV__) { + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'componentDidMount error', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), + expect.stringMatching( + new RegExp( + '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) div(.*)', + ), + ), + expect.stringContaining( + 'Consider adding an error boundary to your tree ' + + 'to customize error handling behavior.', + ), + ); + } else { + expect(console.error).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'componentDidMount error', + }), + ); + } }); it('should ignore errors thrown in log method to prevent cycle', async () => { @@ -135,22 +159,34 @@ describe('ReactIncrementalErrorLogging', () => { ); await waitForThrow('render error'); expect(logCapturedErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls[0]).toEqual( - __DEV__ - ? expect.stringMatching( - new RegExp( - 'The above error occurred in the component:\n' + - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)\n\n' + - 'Consider adding an error boundary to your tree ' + - 'to customize error handling behavior\\.', - ), - ) - : expect.objectContaining({ - message: 'render error', - }), - ); + if (__DEV__) { + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'render error', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), + expect.stringMatching( + new RegExp( + '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) div(.*)', + ), + ), + expect.stringContaining( + 'Consider adding an error boundary to your tree ' + + 'to customize error handling behavior.', + ), + ); + } else { + expect(logCapturedErrorCalls[0]).toEqual( + expect.objectContaining({ + message: 'render error', + }), + ); + } // The error thrown in logCapturedError should be rethrown with a clean stack expect(() => { jest.runAllTimers(); @@ -203,20 +239,31 @@ describe('ReactIncrementalErrorLogging', () => { ); expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error).toHaveBeenCalledWith( - __DEV__ - ? expect.stringMatching( - new RegExp( - 'The above error occurred in the component:\n' + - '\\s+(in|at) Foo (.*)\n' + - '\\s+(in|at) ErrorBoundary (.*)\n\n' + - 'React will try to recreate this component tree from scratch ' + - 'using the error boundary you provided, ErrorBoundary.', - ), - ) - : expect.objectContaining({ - message: 'oops', - }), - ); + if (__DEV__) { + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('%o'), + expect.objectContaining({ + message: 'oops', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), + expect.stringMatching( + new RegExp( + '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', + ), + ), + expect.stringContaining( + 'React will try to recreate this component tree from scratch ' + + 'using the error boundary you provided, ErrorBoundary.', + ), + ); + } else { + expect(console.error).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'oops', + }), + ); + } }); }); diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index 7e5a7b0365549..5d460444440d8 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -608,6 +608,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { ? [ 'Uncaught [', 'The result of getSnapshot should be cached to avoid an infinite loop', + 'The above error occurred in the', ] : [ 'The result of getSnapshot should be cached to avoid an infinite loop', diff --git a/scripts/jest/shouldIgnoreConsoleError.js b/scripts/jest/shouldIgnoreConsoleError.js index 01fe879fcf2b2..2627e927eb892 100644 --- a/scripts/jest/shouldIgnoreConsoleError.js +++ b/scripts/jest/shouldIgnoreConsoleError.js @@ -7,10 +7,13 @@ module.exports = function shouldIgnoreConsoleError( ) { if (__DEV__) { if (typeof format === 'string') { - if (format.indexOf('The above error occurred') === 0) { - // TODO: These errors should be removed. - // This looks like an error addendum from ReactFiberErrorLogger. - // Ignore it too. + if ( + args[0] != null && + typeof args[0].message === 'string' && + typeof args[0].stack === 'string' + ) { + // This looks like an error with addendum from ReactFiberErrorLogger. + // They are noisy too so we'll try to ignore them. return true; } if (