Do not passthrough logbox errors that already have a component stack (#44888)

Summary:
Pull Request resolved: #44888

In facebook/react#29839 we removed the `Warning: ` prefix. This PR replaces the special cases in LogBox for `Warning: ` to use the presence of a component stack instead. This is what LogBox really cares about anyway, since the reason to let errors pass through to the exception manager is to let DevTools add the component stacks.

Changelog: [General] [Fixed] - Fix logbox reporting for React errors

Reviewed By: rickhanlonii

Differential Revision: D58441017

fbshipit-source-id: 5355cd04ddcd5238dadbfcbd64fe1f43c8cd04dc

Author: yungsters

packages/react-native/Libraries/LogBox/Data/LogBoxData.js

@@ -13,6 +13,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 | null,
   stack?: string,
 }>;
 
@@ -215,6 +217,7 @@ export function addLog(log: LogData): void {
           stack,
           category: log.category,
           componentStack: log.componentStack,
+          componentStackType: log.componentStackType || 'legacy',
         }),
       );
     } catch (error) {

packages/react-native/Libraries/LogBox/Data/__tests__/LogBoxData-test.js

@@ -65,6 +65,7 @@ const addLogs = (logs: Array<string>, options: void | {flush: boolean}) => {
       },
       category: message,
       componentStack: [],
+      componentStackType: null,
     });
     if (options == null || options.flush !== false) {
       jest.runOnlyPendingTimers();

packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js

@@ -119,7 +119,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',
@@ -128,18 +128,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,
           },
         ],
       },
@@ -149,7 +149,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)',
       ]),
@@ -168,14 +168,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,
           },
         ],
       },
@@ -805,7 +805,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)',
         ]),
@@ -829,14 +829,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,
             },
           ],
         },
@@ -911,7 +911,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)',
@@ -936,18 +936,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,
             },
           ],
         },
@@ -1080,7 +1080,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)',
         ]),
@@ -1103,14 +1103,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,
             },
           ],
         },
@@ -1183,7 +1183,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)',
@@ -1207,18 +1207,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,
             },
           ],
         },
@@ -1289,7 +1289,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',
         ]),
@@ -1316,14 +1316,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,
             },
           ],
         },
@@ -1488,7 +1488,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',
@@ -1516,18 +1516,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,
             },
           ],
         },

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<mixed>): boolean {
+  for (const arg of args) {
+    if (typeof arg === 'string' && isComponentStack(arg)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 export type ExtendedExceptionData = ExceptionData & {
   isComponentError: boolean,
   ...

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};
 
@@ -150,14 +151,16 @@ if (__DEV__) {
 
     try {
       if (!isRCTLogAdviceWarning(...args)) {
-        const {category, message, componentStack} = parseLogBoxLog(args);
+        const {category, message, componentStack, componentStackType} =
+          parseLogBoxLog(args);
 
         if (!LogBoxData.isMessageIgnored(message.content)) {
           LogBoxData.addLog({
             level: 'warn',
             category,
             message,
             componentStack,
+            componentStackType,
           });
         }
       }
@@ -176,12 +179,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' module, or React errors with
+        // component stacks, otherwise pass the error through.u
+        //
         // 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 +201,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 +232,7 @@ if (__DEV__) {
           category,
           message,
           componentStack,
+          componentStackType,
         });
       }
     } catch (err) {

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);
   });
 });