LogBox - Better error support

Summary:
This diff adds support for thrown exceptions to redboxes, and hides the native redbox when we show an error in LogBox.

Changelog: [Internal]

Reviewed By: cpojer

Differential Revision: D18212064

fbshipit-source-id: 92031d554968bcb079f81568673ae85697c8f5ad

Libraries/Core/ExceptionsManager.js

@@ -11,6 +11,7 @@
 'use strict';
 
 import type {ExtendedError} from './Devtools/parseErrorStack';
+import * as LogBoxData from '../LogBox/Data/LogBoxData';
 import type {ExceptionData} from './NativeExceptionsManager';
 
 class SyntheticError extends Error {
@@ -81,6 +82,14 @@ function reportException(e: ExtendedError, isFatal: boolean) {
     message =
       e.jsEngine == null ? message : `${message}, js engine: ${e.jsEngine}`;
 
+    // TransformErrors need to be popped to the user and can happen both in JS
+    // through Fast Resfresh and through native when reloading a broken bundle.
+    // We want a consistent experience here, so we're opting to always pass
+    // these errors to the native redbox handling.
+    const isHandledByLogBox =
+      !/^TransformError SyntaxError: /.test(originalMessage) &&
+      global.__reactExperimentalLogBox;
+
     const data = preprocessException({
       message,
       originalMessage: message === originalMessage ? null : originalMessage,
@@ -93,9 +102,17 @@ function reportException(e: ExtendedError, isFatal: boolean) {
       extraData: {
         jsEngine: e.jsEngine,
         rawStack: e.stack,
+
+        // Hack to hide native redboxes when in the LogBox experiment.
+        // This is intentionally untyped and stuffed here, because it is temporary.
+        suppressRedBox: isHandledByLogBox,
       },
     });
 
+    if (isHandledByLogBox) {
+      LogBoxData.addException(data);
+    }
+
     NativeExceptionsManager.reportException(data);
 
     if (__DEV__) {
@@ -156,8 +173,13 @@ function reactConsoleErrorHandler() {
   } else {
     console._errorOriginal.apply(console, arguments);
     const stringifySafe = require('../Utilities/stringifySafe');
-    const str = Array.prototype.map.call(arguments, stringifySafe).join(', ');
-    if (str.slice(0, 10) === '"Warning: ') {
+    const str = Array.prototype.map
+      .call(arguments, value =>
+        typeof value === 'string' ? value : stringifySafe(value),
+      )
+      .join(' ');
+
+    if (str.slice(0, 9) === 'Warning: ') {
       // React warnings use console.error so that a stack trace is shown, but
       // we don't (currently) want these to show a redbox
       // (Note: Logic duplicated in polyfills/console.js.)

Libraries/Core/__tests__/ExceptionsManager-test.js

@@ -301,10 +301,8 @@ describe('ExceptionsManager', () => {
 
       expect(nativeReportException.mock.calls.length).toBe(1);
       const exceptionData = nativeReportException.mock.calls[0][0];
-      expect(exceptionData.message).toBe(
-        'console.error: "Some error happened"',
-      );
-      expect(exceptionData.originalMessage).toBe('"Some error happened"');
+      expect(exceptionData.message).toBe('console.error: Some error happened');
+      expect(exceptionData.originalMessage).toBe('Some error happened');
       expect(exceptionData.name).toBe('console.error');
       expect(
         getLineFromFrame(getFirstFrameInThisFile(exceptionData.stack)),
@@ -321,10 +319,10 @@ describe('ExceptionsManager', () => {
       expect(nativeReportException.mock.calls.length).toBe(1);
       const exceptionData = nativeReportException.mock.calls[0][0];
       expect(exceptionData.message).toBe(
-        'console.error: 42, true, ["symbol" failed to stringify], {"y":null}',
+        'console.error: 42 true ["symbol" failed to stringify] {"y":null}',
       );
       expect(exceptionData.originalMessage).toBe(
-        '42, true, ["symbol" failed to stringify], {"y":null}',
+        '42 true ["symbol" failed to stringify] {"y":null}',
       );
       expect(exceptionData.name).toBe('console.error');
       expect(

Libraries/LogBox/Data/LogBoxData.js

@@ -11,9 +11,14 @@
 ('use strict');
 
 import LogBoxLog from './LogBoxLog';
-import parseLogBoxLog from './parseLogBoxLog';
+import {
+  parseLogBoxLog,
+  parseCategory,
+  parseComponentStack,
+} from './parseLogBoxLog';
 import type {LogLevel} from './LogBoxLog';
 import parseErrorStack from '../../Core/Devtools/parseErrorStack';
+import type {ExceptionData} from '../../Core/NativeExceptionsManager';
 
 export type LogBoxLogs = Set<LogBoxLog>;
 
@@ -54,7 +59,7 @@ function handleUpdate(): void {
   }
 }
 
-export function add(level: LogLevel, args: $ReadOnlyArray<mixed>): void {
+export function addLog(level: LogLevel, args: $ReadOnlyArray<mixed>): void {
   const errorForStackTrace = new Error();
 
   // Parsing logs are expensive so we schedule this
@@ -107,6 +112,43 @@ export function symbolicateLogLazy(log: LogBoxLog) {
   log.symbolicate();
 }
 
+export function addException(error: ExceptionData): void {
+  // Parsing logs are expensive so we schedule this
+  // otherwise spammy logs would pause rendering.
+  setImmediate(() => {
+    const {category, message} = parseCategory([
+      error.originalMessage != null ? error.originalMessage : 'Unknown',
+    ]);
+
+    // We don't want to store these logs because they trigger a
+    // state update whenever we add them to the store, which is
+    // expensive to noisy logs. If we later want to display these
+    // we will store them in a different state object.
+    if (isMessageIgnored(message.content)) {
+      return;
+    }
+
+    const lastLog = Array.from(logs).pop();
+    if (lastLog && lastLog.category === category) {
+      lastLog.incrementCount();
+    } else {
+      logs.add(
+        new LogBoxLog(
+          'error',
+          message,
+          error.stack,
+          category,
+          error.componentStack != null
+            ? parseComponentStack(error.componentStack)
+            : [],
+        ),
+      );
+    }
+
+    handleUpdate();
+  });
+}
+
 export function clear(): void {
   if (logs.size > 0) {
     logs.clear();

Libraries/LogBox/Data/__tests__/LogBoxData-test.js

@@ -36,55 +36,71 @@ const observe = () => {
   };
 };
 
-const logAndFlush = logs => {
+const addLogs = logs => {
   logs.forEach(log => {
-    LogBoxData.add(log.level, log.args);
+    LogBoxData.addLog('warn', typeof log === 'string' ? [log] : log);
   });
-
-  jest.runAllImmediates();
 };
 
-const logAndFlushAndUpdate = logs => {
-  logAndFlush(logs);
-
-  // We run immediates again to flush the updates.
-  jest.runAllImmediates();
+const addExceptions = errors => {
+  errors.forEach(error => {
+    LogBoxData.addException(
+      Object.assign(
+        {},
+        {
+          message: '',
+          originalMessage: '',
+          name: 'console.error',
+          componentStack: '',
+          stack: [],
+          id: 0,
+          isFatal: false,
+        },
+        typeof error === 'string'
+          ? {message: error, originalMessage: error}
+          : error,
+      ),
+    );
+  });
 };
+
+beforeEach(() => {
+  jest.resetModules();
+});
+
 describe('LogBoxData', () => {
-  beforeEach(() => {
-    jest.resetModules();
-  });
-
   it('adds and dismisses logs', () => {
-    logAndFlush([{level: 'warn', args: ['A']}]);
+    addLogs(['A']);
+    addExceptions(['B']);
+    jest.runAllImmediates();
 
-    expect(registry().length).toBe(1);
+    expect(registry().length).toBe(2);
     expect(registry()[0]).toBeDefined();
+    expect(registry()[1]).toBeDefined();
 
+    LogBoxData.dismiss(registry()[0]);
+    expect(registry().length).toBe(1);
     LogBoxData.dismiss(registry()[0]);
     expect(registry().length).toBe(0);
     expect(registry()[0]).toBeUndefined();
   });
 
   it('clears all logs', () => {
-    logAndFlush([
-      {level: 'warn', args: ['A']},
-      {level: 'warn', args: ['B']},
-      {level: 'warn', args: ['C']},
-    ]);
+    addLogs(['A', 'B', 'C']);
+    addExceptions(['D']);
+    jest.runAllImmediates();
 
-    expect(registry().length).toBe(3);
+    expect(registry().length).toBe(4);
 
     LogBoxData.clear();
     expect(registry().length).toBe(0);
   });
 
   it('keeps logs in chronological order', () => {
-    logAndFlush([
-      {level: 'warn', args: ['A']},
-      {level: 'warn', args: ['B']},
-      {level: 'warn', args: ['C']},
-    ]);
+    addLogs(['A']);
+    addExceptions(['B']);
+    addLogs(['C']);
+    jest.runAllImmediates();
 
     let logs = registry();
     expect(logs.length).toBe(3);
@@ -92,7 +108,8 @@ describe('LogBoxData', () => {
     expect(logs[1].category).toEqual('B');
     expect(logs[2].category).toEqual('C');
 
-    logAndFlush([{level: 'warn', args: ['A']}]);
+    addLogs(['A']);
+    jest.runAllImmediates();
 
     // Expect `A` to be added to the end of the registry.
     logs = registry();
@@ -103,8 +120,9 @@ describe('LogBoxData', () => {
     expect(logs[3].category).toEqual('A');
   });
 
-  it('increments the count of previous log with matching category', () => {
-    logAndFlush([{level: 'warn', args: ['A']}, {level: 'warn', args: ['B']}]);
+  it('increments the count of previous log with matching category (logs)', () => {
+    addLogs(['A', 'B']);
+    jest.runAllImmediates();
 
     let logs = registry();
     expect(logs.length).toBe(2);
@@ -113,7 +131,8 @@ describe('LogBoxData', () => {
     expect(logs[1].category).toEqual('B');
     expect(logs[1].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['B']}]);
+    addLogs(['B']);
+    jest.runAllImmediates();
 
     // Expect `B` to be rolled into the last log.
     logs = registry();
@@ -124,12 +143,33 @@ describe('LogBoxData', () => {
     expect(logs[1].count).toBe(2);
   });
 
-  it('ignores logs matching patterns', () => {
-    logAndFlush([
-      {level: 'warn', args: ['A!']},
-      {level: 'warn', args: ['B?']},
-      {level: 'warn', args: ['C!']},
-    ]);
+  it('increments the count of previous log with matching category (exceptions)', () => {
+    addExceptions(['A', 'B']);
+    jest.runAllImmediates();
+
+    let logs = registry();
+    expect(logs.length).toBe(2);
+    expect(logs[0].category).toEqual('A');
+    expect(logs[0].count).toBe(1);
+    expect(logs[1].category).toEqual('B');
+    expect(logs[1].count).toBe(1);
+
+    addExceptions(['B']);
+    jest.runAllImmediates();
+
+    // Expect `B` to be rolled into the last log.
+    logs = registry();
+    expect(logs.length).toBe(2);
+    expect(logs[0].category).toEqual('A');
+    expect(logs[0].count).toBe(1);
+    expect(logs[1].category).toEqual('B');
+    expect(logs[1].count).toBe(2);
+  });
+
+  it('ignores logs matching patterns (logs)', () => {
+    addLogs(['A!', 'B?', 'C!']);
+    jest.runAllImmediates();
+
     expect(filteredRegistry().length).toBe(3);
 
     LogBoxData.addIgnorePatterns(['!']);
@@ -139,12 +179,38 @@ describe('LogBoxData', () => {
     expect(filteredRegistry().length).toBe(0);
   });
 
-  it('ignores logs matching regexs or pattern', () => {
-    logAndFlush([
-      {level: 'warn', args: ['There are 4 dogs']},
-      {level: 'warn', args: ['There are 3 cats']},
-      {level: 'warn', args: ['There are H cats']},
-    ]);
+  it('ignores logs matching patterns (exceptions)', () => {
+    addExceptions(['A!', 'B?', 'C!']);
+    jest.runAllImmediates();
+
+    expect(filteredRegistry().length).toBe(3);
+
+    LogBoxData.addIgnorePatterns(['!']);
+    expect(filteredRegistry().length).toBe(1);
+
+    LogBoxData.addIgnorePatterns(['?']);
+    expect(filteredRegistry().length).toBe(0);
+  });
+
+  it('ignores matching regexs or pattern (logs)', () => {
+    addLogs(['There are 4 dogs', 'There are 3 cats', 'There are H cats']);
+    jest.runAllImmediates();
+
+    expect(filteredRegistry().length).toBe(3);
+
+    LogBoxData.addIgnorePatterns(['dogs']);
+    expect(filteredRegistry().length).toBe(2);
+
+    LogBoxData.addIgnorePatterns([/There are \d+ cats/]);
+    expect(filteredRegistry().length).toBe(1);
+
+    LogBoxData.addIgnorePatterns(['cats']);
+    expect(filteredRegistry().length).toBe(0);
+  });
+
+  it('ignores matching regexs or pattern (exceptions)', () => {
+    addExceptions(['There are 4 dogs', 'There are 3 cats', 'There are H cats']);
+    jest.runAllImmediates();
 
     expect(filteredRegistry().length).toBe(3);
 
@@ -159,11 +225,10 @@ describe('LogBoxData', () => {
   });
 
   it('ignores all logs when disabled', () => {
-    logAndFlush([
-      {level: 'warn', args: ['A!']},
-      {level: 'warn', args: ['B?']},
-      {level: 'warn', args: ['C!']},
-    ]);
+    addLogs(['A!', 'B?']);
+    addExceptions(['C!']);
+    jest.runAllImmediates();
+
     expect(registry().length).toBe(3);
 
     LogBoxData.setDisabled(true);
@@ -174,57 +239,68 @@ describe('LogBoxData', () => {
   });
 
   it('groups consecutive logs by format string categories', () => {
-    logAndFlush([{level: 'warn', args: ['%s', 'A']}]);
-
+    addLogs([['%s', 'A']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(1);
     expect(registry()[0].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['%s', 'B']}]);
+    addLogs([['%s', 'B']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(1);
     expect(registry()[0].count).toBe(2);
 
-    logAndFlush([{level: 'warn', args: ['A']}]);
+    addLogs(['A']);
+    jest.runAllImmediates();
     expect(registry().length).toBe(2);
     expect(registry()[1].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['B']}]);
+    addLogs(['B']);
+    jest.runAllImmediates();
     expect(registry().length).toBe(3);
     expect(registry()[2].count).toBe(1);
   });
 
   it('groups warnings with consideration for arguments', () => {
-    logAndFlush([{level: 'warn', args: ['A', 'B']}]);
+    addLogs([['A', 'B']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(1);
     expect(registry()[0].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['A', 'B']}]);
+    addLogs([['A', 'B']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(1);
     expect(registry()[0].count).toBe(2);
 
-    logAndFlush([{level: 'warn', args: ['A', 'C']}]);
+    addLogs([['A', 'C']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(2);
     expect(registry()[1].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['%s', 'A', 'A']}]);
+    addLogs([['%s', 'A', 'A']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(3);
     expect(registry()[2].count).toBe(1);
 
-    logAndFlush([{level: 'warn', args: ['%s', 'B', 'A']}]);
+    addLogs([['%s', 'B', 'A']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(3);
     expect(registry()[2].count).toBe(2);
 
-    logAndFlush([{level: 'warn', args: ['%s', 'B', 'B']}]);
+    addLogs([['%s', 'B', 'B']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(4);
     expect(registry()[3].count).toBe(1);
   });
 
   it('ignores logs starting with "(ADVICE)"', () => {
-    logAndFlush([{level: 'warn', args: ['(ADVICE) ...']}]);
+    addLogs(['(ADVICE) ...']);
+    jest.runAllImmediates();
     expect(registry().length).toBe(0);
   });
 
   it('does not ignore logs formatted to start with "(ADVICE)"', () => {
-    logAndFlush([{level: 'warn', args: ['%s ...', '(ADVICE)']}]);
+    addLogs([['%s ...', '(ADVICE)']]);
+    jest.runAllImmediates();
     expect(registry().length).toBe(1);
   });
 
@@ -243,10 +319,8 @@ describe('LogBoxData', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    logAndFlushAndUpdate([
-      {level: 'warn', args: ['A']},
-      {level: 'warn', args: ['B']},
-    ]);
+    addLogs(['A']);
+    jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
 
     // We expect observers to recieve the same Set object in sequential updates
@@ -270,7 +344,8 @@ describe('LogBoxData', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    logAndFlushAndUpdate([{level: 'warn', args: ['A']}]);
+    addLogs(['A']);
+    jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
 
     const lastLog = Array.from(observer.mock.calls[1][0])[0];
@@ -288,7 +363,8 @@ describe('LogBoxData', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    logAndFlushAndUpdate([{level: 'warn', args: ['A']}]);
+    addLogs(['A']);
+    jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
 
     LogBoxData.clear();

Libraries/LogBox/Data/__tests__/LogBoxLogParser-test.js

@@ -15,7 +15,7 @@ jest.mock('../../../Core/Devtools/parseErrorStack', () => {
   return {__esModule: true, default: jest.fn(() => [])};
 });
 
-const parseLogBoxLog = require('../parseLogBoxLog').default;
+const {parseLogBoxLog} = require('../parseLogBoxLog');
 
 describe('parseLogBoxLog', () => {
   it('parses strings', () => {

Libraries/LogBox/Data/parseLogBoxLog.js

@@ -34,7 +34,7 @@ export type ComponentStack = $ReadOnlyArray<
 
 const SUBSTITUTION = UTFSequence.BOM + '%s';
 
-function parseCategory(
+export function parseCategory(
   args: $ReadOnlyArray<mixed>,
 ): $ReadOnly<{|
   category: Category,
@@ -110,8 +110,23 @@ function parseCategory(
     },
   };
 }
+export function parseComponentStack(message: string): ComponentStack {
+  return message
+    .split(/\n {4}in /g)
+    .map(s => {
+      if (!s) {
+        return null;
+      }
+      let [component, location] = s.split(/ \(at /);
+      if (!location) {
+        [component, location] = s.split(/ \(/);
+      }
+      return {component, location: location && location.replace(')', '')};
+    })
+    .filter(Boolean);
+}
 
-function parseLog(
+export function parseLogBoxLog(
   args: $ReadOnlyArray<mixed>,
 ): {|
   componentStack: ComponentStack,
@@ -129,19 +144,7 @@ function parseLog(
   let argsWithoutComponentStack = [];
   for (const arg of args) {
     if (typeof arg === 'string' && /^\n {4}in/.exec(arg)) {
-      componentStack = arg
-        .split(/\n {4}in /g)
-        .map(s => {
-          if (!s) {
-            return null;
-          }
-          let [component, location] = s.split(/ \(at /);
-          if (!location) {
-            [component, location] = s.split(/ \(/);
-          }
-          return {component, location: location && location.replace(')', '')};
-        })
-        .filter(Boolean);
+      componentStack = parseComponentStack(arg);
     } else {
       argsWithoutComponentStack.push(arg);
     }
@@ -152,5 +155,3 @@ function parseLog(
     componentStack,
   };
 }
-
-export default parseLog;

Libraries/LogBox/LogBox.js

@@ -60,8 +60,6 @@ if (__DEV__) {
         // Show LogBox for the `warning` module.
         if (typeof args[0] === 'string' && args[0].startsWith('Warning: ')) {
           registerWarning(...args);
-        } else {
-          registerError(...args);
         }
       };
 
@@ -133,11 +131,7 @@ if (__DEV__) {
   };
 
   const registerWarning = (...args): void => {
-    LogBoxData.add('warn', args);
-  };
-
-  const registerError = (...args): void => {
-    LogBoxData.add('error', args);
+    LogBoxData.addLog('warn', args);
   };
 } else {
   LogBoxComponent = class extends React.Component<Props, State> {

Libraries/LogBox/UI/LogBoxMessage.js

@@ -21,7 +21,7 @@ type Props = {
   style: TextStyleProp,
 };
 
-const cleanContent = content => content.replace(/Warning: /g, '');
+const cleanContent = content => content.replace(/(Warning|Error): /g, '');
 
 function LogBoxMessage(props: Props): React.Node {
   const {content, substitutions}: Message = props.message;

Libraries/LogBox/__tests__/LogBox-test.js

@@ -60,9 +60,9 @@ describe('LogBox', () => {
 
     LogBox.install();
 
-    expect(LogBoxData.add).not.toBeCalled();
+    expect(LogBoxData.addLog).not.toBeCalled();
     console.warn('...');
-    expect(LogBoxData.add).toBeCalled();
+    expect(LogBoxData.addLog).toBeCalled();
   });
 
   it('registers errors beginning with "Warning: " as warnings', () => {
@@ -71,9 +71,9 @@ describe('LogBox', () => {
     LogBox.install();
 
     console.error('...');
-    expect(LogBoxData.add).toBeCalledWith('error', ['...']);
+    expect(LogBoxData.addLog).not.toBeCalled();
 
     console.error('Warning: ...');
-    expect(LogBoxData.add).toBeCalledWith('warn', ['Warning: ...']);
+    expect(LogBoxData.addLog).toBeCalledWith('warn', ['Warning: ...']);
   });
 });