From b3469700169b5c90d5957f303294610b40f4f5ba Mon Sep 17 00:00:00 2001
From: Rick Hanlon <rickhanlonii@fb.com>
Date: Mon, 18 Nov 2019 13:45:59 -0800
Subject: [PATCH] LogBox - Minor error message formatting fixes

Summary:
Adds better handling of component stacks for warnings and clarifies why we ignore `(ADVICE)` warnings.

Changelog: [Internal]

Reviewed By: cpojer

Differential Revision: D18575693

fbshipit-source-id: f4eacbf74c62cd079f2c441951849fb03e0941dd
---
 .../Data/__tests__/parseLogBoxLog-test.js     | 27 +++++++++++++
 Libraries/LogBox/Data/parseLogBoxLog.js       | 39 ++++++++++++-------
 Libraries/LogBox/LogBox.js                    | 17 ++++++--
 3 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js b/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js
index b4d311e817..11ee0b991c 100644
--- a/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js
+++ b/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js
@@ -108,6 +108,33 @@ 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',
+        '\n\nCheck the render method of `Container(Component)`.',
+        '\n    in MyComponent (at filename.js:1)\n    in MyOtherComponent (at filename2.js:1)',
+      ]),
+    ).toEqual({
+      componentStack: [
+        {component: 'MyComponent', location: 'filename.js:1'},
+        {component: 'MyOtherComponent', location: 'filename2.js:1'},
+      ],
+      category:
+        'Warning: 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)`.',
+        substitutions: [
+          {
+            length: 52,
+            offset: 129,
+          },
+        ],
+      },
+    });
+  });
+
   it('detects a component stack in the second argument', () => {
     expect(
       parseLogBoxLog([
diff --git a/Libraries/LogBox/Data/parseLogBoxLog.js b/Libraries/LogBox/Data/parseLogBoxLog.js
index 9ddd67eaa3..96e0a219cf 100644
--- a/Libraries/LogBox/Data/parseLogBoxLog.js
+++ b/Libraries/LogBox/Data/parseLogBoxLog.js
@@ -187,20 +187,33 @@ export function parseLogBoxLog(
   category: Category,
   message: Message,
 |} {
-  // This detects a very narrow case of a simple log string,
-  // with a component stack appended by React DevTools.
-  // In this case, we extract the component stack,
-  // because LogBox formats those pleasantly.
-  // If there are other substitutions or formatting,
-  // this could potentially corrupt the data, but there
-  // are currently no known cases of that happening.
-  let componentStack = [];
+  const message = args[0];
   let argsWithoutComponentStack = [];
-  for (const arg of args) {
-    if (typeof arg === 'string' && /^\n {4}in/.exec(arg)) {
-      componentStack = parseComponentStack(arg);
-    } else {
-      argsWithoutComponentStack.push(arg);
+  let componentStack = [];
+
+  // Extract component stack from warnings like "Some warning%s".
+  if (
+    typeof message === 'string' &&
+    message.slice(-2) === '%s' &&
+    args.length > 0
+  ) {
+    const lastArg = args[args.length - 1];
+    // Does it look like React component stack? "   in ..."
+    if (typeof lastArg === 'string' && /\s{4}in/.test(lastArg)) {
+      argsWithoutComponentStack = args.slice(0, -1);
+      argsWithoutComponentStack[0] = message.slice(0, -2);
+      componentStack = parseComponentStack(lastArg);
+    }
+  }
+
+  if (componentStack.length === 0) {
+    // Try finding the component stack elsewhere.
+    for (const arg of args) {
+      if (typeof arg === 'string' && /^\n {4}in/.exec(arg)) {
+        componentStack = parseComponentStack(arg);
+      } else {
+        argsWithoutComponentStack.push(arg);
+      }
     }
   }
 
diff --git a/Libraries/LogBox/LogBox.js b/Libraries/LogBox/LogBox.js
index 99b14e5c27..0a56ddb4f9 100644
--- a/Libraries/LogBox/LogBox.js
+++ b/Libraries/LogBox/LogBox.js
@@ -150,10 +150,19 @@ if (__DEV__) {
     }
   };
 
+  const isRCTLogAdviceWarning = (...args) => {
+    // RCTLogAdvice is a native logging function designed to show users
+    // a message in the console, but not show it to them in Logbox.
+    return typeof args[0] === 'string' && args[0].startsWith('(ADVICE)');
+  };
+
+  const isWarningModuleWarning = (...args) => {
+    return typeof args[0] === 'string' && args[0].startsWith('Warning: ');
+  };
+
   const registerWarning = (...args): void => {
     try {
-      // This is carried over from the old YellowBox, but it is not clear why.
-      if (typeof args[0] !== 'string' || !args[0].startsWith('(ADVICE)')) {
+      if (!isRCTLogAdviceWarning(...args)) {
         const {category, message, componentStack} = parseLogBoxLog(args);
 
         if (!LogBoxData.isMessageIgnored(message.content)) {
@@ -177,8 +186,8 @@ if (__DEV__) {
 
   const registerError = (...args): void => {
     try {
-      // Only show LogBox for the `warning` module, otherwise pass through and skip.
-      if (typeof args[0] !== 'string' || !args[0].startsWith('Warning: ')) {
+      if (!isWarningModuleWarning(...args)) {
+        // Only show LogBox for the `warning` module, otherwise pass through and skip.
         error.call(console, ...args);
         return;
       }