diff --git a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js index 34b1d5cb56d593..462c1094245602 100644 --- a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js +++ b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js @@ -15,6 +15,7 @@ import type {LogLevel} from './LogBoxLog'; import type { Category, ComponentStack, + ComponentStackType, ExtendedExceptionData, Message, } from './parseLogBoxLog'; @@ -30,6 +31,7 @@ export type LogData = $ReadOnly<{| message: Message, category: Category, componentStack: ComponentStack, + componentStackType?: ComponentStackType, stack?: string, |}>; @@ -209,6 +211,7 @@ export function addLog(log: LogData): void { stack, category: log.category, componentStack: log.componentStack, + componentStackType: log.componentStackType, }), ); } catch (error) { diff --git a/packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js b/packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js index 8de9a84f3266ec..0eda97022868b0 100644 --- a/packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js +++ b/packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js @@ -115,7 +115,7 @@ describe('parseLogBoxLog', () => { it('does not duplicate message if component stack found but not parsed', () => { expect( parseLogBoxLog([ - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', '\n\nCheck the render method of `MyOtherComponent`.', '', '\n in\n in\n in', @@ -124,18 +124,18 @@ describe('parseLogBoxLog', () => { componentStackType: 'legacy', componentStack: [], category: - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', message: { content: - 'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', substitutions: [ { length: 48, - offset: 62, + offset: 53, }, { length: 0, - offset: 110, + offset: 101, }, ], }, @@ -145,7 +145,7 @@ describe('parseLogBoxLog', () => { it('detects a component stack in an interpolated warning', () => { expect( parseLogBoxLog([ - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', '\n\nCheck the render method of `Container(Component)`.', '\n in MyComponent (at filename.js:1)\n in MyOtherComponent (at filename2.js:1)', ]), @@ -164,14 +164,14 @@ describe('parseLogBoxLog', () => { }, ], category: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', message: { content: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `Container(Component)`.', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `Container(Component)`.', substitutions: [ { length: 52, - offset: 129, + offset: 120, }, ], }, @@ -801,7 +801,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in an interpolated warning', () => { expect( parseLogBoxLog([ - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', '\n\nCheck the render method of `MyComponent`.', '\n in MyComponent (created by MyOtherComponent)\n in MyOtherComponent (created by MyComponent)\n in MyAppComponent (created by MyOtherComponent)', ]), @@ -825,14 +825,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', message: { content: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', substitutions: [ { length: 43, - offset: 129, + offset: 120, }, ], }, @@ -907,7 +907,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in the nth argument', () => { expect( parseLogBoxLog([ - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', '\n\nCheck the render method of `MyOtherComponent`.', '', '\n in MyComponent (created by MyOtherComponent)\n in MyOtherComponent (created by MyComponent)\n in MyAppComponent (created by MyOtherComponent)', @@ -932,18 +932,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', message: { content: - 'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', substitutions: [ { length: 48, - offset: 62, + offset: 53, }, { length: 0, - offset: 110, + offset: 101, }, ], }, @@ -1076,7 +1076,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in an interpolated warning', () => { expect( parseLogBoxLog([ - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', '\n\nCheck the render method of `MyComponent`.', '\n at MyComponent (/path/to/filename.js:1:2)\n at MyOtherComponent\n at MyAppComponent (/path/to/app.js:100:20)', ]), @@ -1099,14 +1099,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', message: { content: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', substitutions: [ { length: 43, - offset: 129, + offset: 120, }, ], }, @@ -1179,7 +1179,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in the nth argument', () => { expect( parseLogBoxLog([ - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', '\n\nCheck the render method of `MyOtherComponent`.', '', '\n at MyComponent (/path/to/filename.js:1:2)\n at MyOtherComponent\n at MyAppComponent (/path/to/app.js:100:20)', @@ -1203,18 +1203,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', message: { content: - 'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', substitutions: [ { length: 48, - offset: 62, + offset: 53, }, { length: 0, - offset: 110, + offset: 101, }, ], }, @@ -1285,7 +1285,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in an interpolated warning', () => { expect( parseLogBoxLog([ - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s', '\n\nCheck the render method of `MyComponent`.', '\nMyComponent@/path/to/filename.js:1:2\nforEach@[native code]\nMyAppComponent@/path/to/app.js:100:20', ]), @@ -1312,14 +1312,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s', message: { content: - 'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', + 'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.', substitutions: [ { length: 43, - offset: 129, + offset: 120, }, ], }, @@ -1484,7 +1484,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, it('detects a component stack in the nth argument', () => { expect( parseLogBoxLog([ - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s', '\n\nCheck the render method of `MyOtherComponent`.', '', '\nMyComponent@/path/to/filename.js:1:2\nforEach@[native code]\nMyAppComponent@/path/to/app.js:100:20', @@ -1512,18 +1512,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`, }, ], category: - 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.', message: { content: - 'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', + 'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.', substitutions: [ { length: 48, - offset: 62, + offset: 53, }, { length: 0, - offset: 110, + offset: 101, }, ], }, diff --git a/packages/react-native/Libraries/LogBox/Data/parseLogBoxLog.js b/packages/react-native/Libraries/LogBox/Data/parseLogBoxLog.js index af0bb5c20101dd..6f9adecbf1c038 100644 --- a/packages/react-native/Libraries/LogBox/Data/parseLogBoxLog.js +++ b/packages/react-native/Libraries/LogBox/Data/parseLogBoxLog.js @@ -93,6 +93,15 @@ const RE_BABEL_CODE_FRAME_MARKER_PATTERN = new RegExp( 'm', ); +export function hasComponentStack(args: $ReadOnlyArray): boolean { + for (const arg of args) { + if (typeof arg === 'string' && isComponentStack(arg)) { + return true; + } + } + return false; +} + export type ExtendedExceptionData = ExceptionData & { isComponentError: boolean, ... diff --git a/packages/react-native/Libraries/LogBox/LogBox.js b/packages/react-native/Libraries/LogBox/LogBox.js index 22c7596fe063b9..36a78b449bb174 100644 --- a/packages/react-native/Libraries/LogBox/LogBox.js +++ b/packages/react-native/Libraries/LogBox/LogBox.js @@ -13,6 +13,7 @@ import type {ExtendedExceptionData} from './Data/parseLogBoxLog'; import Platform from '../Utilities/Platform'; import RCTLog from '../Utilities/RCTLog'; +import {hasComponentStack} from './Data/parseLogBoxLog'; export type {LogData, ExtendedExceptionData, IgnorePattern}; @@ -176,12 +177,20 @@ if (__DEV__) { } try { - if (!isWarningModuleWarning(...args)) { - // Only show LogBox for the 'warning' module, otherwise pass through. + if (!isWarningModuleWarning(...args) && !hasComponentStack(args)) { + // Only show LogBox for the 'warning' modulee, or React errors with + // component stacks, otherwise pass the error through. + // // By passing through, this will get picked up by the React console override, // potentially adding the component stack. React then passes it back to the // React Native ExceptionsManager, which reports it to LogBox as an error. // + // Ideally, we refactor all RN error handling so that LogBox patching + // errors is not necessary, and they are reported the same as a framework. + // The blocker to this is that the ExceptionManager console.error override + // strigifys all of the args before passing it through to LogBox, which + // would lose all of the interpolation information. + // // The 'warning' module needs to be handled here because React internally calls // `console.error('Warning: ')` with the component stack already included. originalConsoleError(...args); @@ -190,20 +199,25 @@ if (__DEV__) { const format = args[0].replace('Warning: ', ''); const filterResult = LogBoxData.checkWarningFilter(format); - if (filterResult.suppressCompletely) { - return; - } - let level = 'error'; - if (filterResult.suppressDialog_LEGACY === true) { - level = 'warn'; - } else if (filterResult.forceDialogImmediately === true) { - level = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws. + if (filterResult.monitorEvent !== 'warning_unhandled') { + if (filterResult.suppressCompletely) { + return; + } + + if (filterResult.suppressDialog_LEGACY === true) { + level = 'warn'; + } else if (filterResult.forceDialogImmediately === true) { + level = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws. + } } // Unfortunately, we need to add the Warning: prefix back for downstream dependencies. + // Downstream, we check for this prefix to know that LogBox already handled it, so + // it doesn't get reported back to LogBox. It's an absolute mess. args[0] = `Warning: ${filterResult.finalFormat}`; - const {category, message, componentStack} = parseLogBoxLog(args); + const {category, message, componentStack, componentStackType} = + parseLogBoxLog(args); // Interpolate the message so they are formatted for adb and other CLIs. // This is different than the message.content above because it includes component stacks. @@ -216,6 +230,7 @@ if (__DEV__) { category, message, componentStack, + componentStackType, }); } } catch (err) { diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index 2385f352988dbb..07ea7507d77752 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -95,6 +95,8 @@ describe.skip('LogBox', () => { expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot( 'Log passed to console error', ); + + // The Warning: prefix is added due to a hack in LogBox to prevent double logging. expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true); }); @@ -123,6 +125,8 @@ describe.skip('LogBox', () => { expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot( 'Log passed to console error', ); + + // The Warning: prefix is added due to a hack in LogBox to prevent double logging. expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true); }); }); diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js index 70c0ca496e1f85..41e35a64ee2955 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js @@ -132,6 +132,102 @@ describe('LogBox', () => { expect(LogBoxData.checkWarningFilter).not.toBeCalled(); }); + it('registers react errors with the formatting from filter', () => { + jest.mock('../Data/LogBoxData'); + + mockFilterResult({ + finalFormat: 'Custom format', + }); + + LogBox.install(); + + console.error( + 'Each child in a list should have a unique key %s', + '\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({ + message: {content: 'Warning: Custom format', substitutions: []}, + category: 'Warning: Custom format', + }), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith( + 'Each child in a list should have a unique key %s', + ); + }); + + it('registers errors with component stack as errors by default', () => { + jest.mock('../Data/LogBoxData'); + + mockFilterResult({}); + + LogBox.install(); + + console.error( + 'HIT %s', + '\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith('HIT %s'); + }); + + it('registers errors with component stack as errors by default if not found in warning filter', () => { + jest.mock('../Data/LogBoxData'); + + mockFilterResult({ + monitorEvent: 'warning_unhandled', + }); + + LogBox.install(); + + console.error( + 'HIT %s', + '\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith('HIT %s'); + }); + + it('registers errors with component stack with legacy suppression as warning', () => { + jest.mock('../Data/LogBoxData'); + + mockFilterResult({ + suppressDialog_LEGACY: true, + }); + + LogBox.install(); + + console.error( + 'Legacy warn %s', + '\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'warn'}), + ); + }); + + it('registers errors with component stack and a forced dialog as fatals', () => { + jest.mock('../Data/LogBoxData'); + + mockFilterResult({ + forceDialogImmediately: true, + }); + + LogBox.install(); + + console.error( + 'Fatal %s', + '\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'fatal'}), + ); + }); + it('registers warning module errors with the formatting from filter', () => { jest.mock('../Data/LogBoxData');