Build Component Stacks from Native Stack Frames (#18561)

* Implement component stack extraction hack * Normalize errors in tests This drops the requirement to include owner to pass the test. * Special case tests * Add destructuring to force toObject which throws before the side-effects This ensures that we don't double call yieldValue or advanceTime in tests. Ideally we could use empty destructuring but ES lint doesn't like it. * Cache the result in DEV In DEV it's somewhat likely that we'll see many logs that add component stacks. This could be slow so we cache the results of previous components. * Fixture * Add Reflect to lint * Log if out of range. * Fix special case when the function call throws in V8 In V8 we need to ignore the first line. Normally we would never get there because the stacks would differ before that, but the stacks are the same if we end up throwing at the same place as the control.
facebook · Apr 10, 2020 · 98d410f · 98d410f
1 parent 8e13f09
commit 98d410f
Show file tree

Hide file tree

Showing 35 changed files with 594 additions and 179 deletions.
diff --git a/fixtures/stacks/BabelClass-compiled.js b/fixtures/stacks/BabelClass-compiled.js
diff --git a/fixtures/stacks/BabelClass-compiled.js.map b/fixtures/stacks/BabelClass-compiled.js.map
diff --git a/fixtures/stacks/BabelClass.js b/fixtures/stacks/BabelClass.js
@@ -0,0 +1,8 @@
+// Compile this with Babel.
+// babel --config-file ./babel.config.json BabelClass.js --out-file BabelClass-compiled.js --source-maps
+
+class BabelClass extends React.Component {
+  render() {
+    return this.props.children;
+  }
+}
diff --git a/fixtures/stacks/Component.js b/fixtures/stacks/Component.js
@@ -0,0 +1,20 @@
+// Example
+
+const Throw = React.lazy(() => {
+  throw new Error('Example');
+});
+
+const Component = React.memo(function Component({children}) {
+  return children;
+});
+
+function DisplayName({children}) {
+  return children;
+}
+DisplayName.displayName = 'Custom Name';
+
+class NativeClass extends React.Component {
+  render() {
+    return this.props.children;
+  }
+}
diff --git a/fixtures/stacks/Example.js b/fixtures/stacks/Example.js
@@ -0,0 +1,59 @@
+// Example
+
+const x = React.createElement;
+
+class ErrorBoundary extends React.Component {
+  static getDerivedStateFromError(error) {
+    return {
+      error: error,
+    };
+  }
+
+  componentDidCatch(error, errorInfo) {
+    console.log(error.message, errorInfo.componentStack);
+    this.setState({
+      componentStack: errorInfo.componentStack,
+    });
+  }
+
+  render() {
+    if (this.state && this.state.error) {
+      return x(
+        'div',
+        null,
+        x('h3', null, this.state.error.message),
+        x('pre', null, this.state.componentStack)
+      );
+    }
+    return this.props.children;
+  }
+}
+
+function Example() {
+  let state = React.useState(false);
+  return x(
+    ErrorBoundary,
+    null,
+    x(
+      DisplayName,
+      null,
+      x(
+        React.SuspenseList,
+        null,
+        x(
+          NativeClass,
+          null,
+          x(
+            BabelClass,
+            null,
+            x(
+              React.Suspense,
+              null,
+              x('div', null, x(Component, null, x(Throw)))
+            )
+          )
+        )
+      )
+    )
+  );
+}
diff --git a/fixtures/stacks/babel.config.json b/fixtures/stacks/babel.config.json
@@ -0,0 +1,5 @@
+{
+  "plugins": [
+    ["@babel/plugin-transform-classes", {"loose": true}]
+  ]
+}
diff --git a/fixtures/stacks/index.html b/fixtures/stacks/index.html
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Component Stacks</title>
+    <style>
+      html, body {
+        margin: 20px;
+      }
+      pre {
+        background: #eee;
+        border: 1px solid #ccc;
+        padding: 2px;
+      }
+    </style>
+  </head>
+  <body>
+    <div id="container">
+      <p>
+        To install React, follow the instructions on
+        <a href="https://github.com/facebook/react/">GitHub</a>.
+      </p>
+      <p>
+        If you can see this, React is <strong>not</strong> working right.
+        If you checked out the source from GitHub make sure to run <code>npm run build</code>.
+      </p>
+    </div>
+    <script src="../../build/node_modules/react/umd/react.production.min.js"></script>
+    <script src="../../build/node_modules/react-dom/umd/react-dom.production.min.js"></script>
+    <script src="./Component.js"></script>
+    <script src="./BabelClass-compiled.js"></script>
+    <script src="./Example.js"></script>
+    <script>
+      const container = document.getElementById("container");
+      ReactDOM.render(React.createElement(Example), container);
+    </script>
+    <h3>The above stack should look something like this:</h3>
+    <pre>
+
+    at Lazy
+    at Component (/stacks/Component.js:7:50)
+    at div
+    at Suspense
+    at BabelClass (/stacks/BabelClass-compiled.js:13:29)
+    at NativeClass (/stacks/Component.js:16:1)
+    at SuspenseList
+    at Custom Name (/stacks/Component.js:11:23)
+    at ErrorBoundary (/stacks/Example.js:5:1)
+    at Example (/stacks/Example.js:33:21)</pre>
+  </body>
+</html>
diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js
@@ -61,7 +61,12 @@ describe('console', () => {
   });
 
   function normalizeCodeLocInfo(str) {
-    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+    return (
+      str &&
+      str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
+        return '\n    in ' + name + ' (at **)';
+      })
+    );
   }
 
   it('should not patch console methods that do not receive component stacks', () => {

diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js
@@ -16,7 +16,12 @@ let ReactTestUtils;
 
 describe('ReactComponent', () => {
   function normalizeCodeLocInfo(str) {
-    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+    return (
+      str &&
+      str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
+        return '\n    in ' + name + ' (at **)';
+      })
+    );
   }
 
   beforeEach(() => {

diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -17,7 +17,12 @@ describe('ReactDOMComponent', () => {
   const ReactFeatureFlags = require('shared/ReactFeatureFlags');
 
   function normalizeCodeLocInfo(str) {
-    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+    return (
+      str &&
+      str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
+        return '\n    in ' + name + ' (at **)';
+      })
+    );
   }
 
   beforeEach(() => {
@@ -1719,16 +1724,26 @@ describe('ReactDOMComponent', () => {
             <tr />
           </div>,
         );
-      }).toErrorDev([
-        'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
-          '<div>.' +
-          '\n    in tr (at **)' +
-          '\n    in div (at **)',
-        'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
-          '<div>.' +
-          '\n    in tr (at **)' +
-          '\n    in div (at **)',
-      ]);
+      }).toErrorDev(
+        ReactFeatureFlags.enableComponentStackLocations
+          ? [
+              // This warning dedupes since they're in the same component.
+              'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
+                '<div>.' +
+                '\n    in tr (at **)' +
+                '\n    in div (at **)',
+            ]
+          : [
+              'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
+                '<div>.' +
+                '\n    in tr (at **)' +
+                '\n    in div (at **)',
+              'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
+                '<div>.' +
+                '\n    in tr (at **)' +
+                '\n    in div (at **)',
+            ],
+      );
     });
 
     it('warns on invalid nesting at root', () => {

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
@@ -1097,7 +1097,7 @@ describe('ReactDOMServerHooks', () => {
 
       it('useOpaqueIdentifier: ID is not used during hydration but is used in an update', async () => {
         let _setShow;
-        function App() {
+        function App({unused}) {
           Scheduler.unstable_yieldValue('App');
           const id = useOpaqueIdentifier();
           const [show, setShow] = useState(false);
@@ -1129,7 +1129,7 @@ describe('ReactDOMServerHooks', () => {
 
       it('useOpaqueIdentifier: ID is not used during hydration but is used in an update in legacy', async () => {
         let _setShow;
-        function App() {
+        function App({unused}) {
           Scheduler.unstable_yieldValue('App');
           const id = useOpaqueIdentifier();
           const [show, setShow] = useState(false);

diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
@@ -497,18 +497,18 @@ describe('ReactErrorBoundaries', () => {
       }
     };
 
-    BrokenUseEffect = props => {
+    BrokenUseEffect = ({children}) => {
       Scheduler.unstable_yieldValue('BrokenUseEffect render');
 
       React.useEffect(() => {
         Scheduler.unstable_yieldValue('BrokenUseEffect useEffect [!]');
         throw new Error('Hello');
       });
 
-      return props.children;
+      return children;
     };
 
-    BrokenUseLayoutEffect = props => {
+    BrokenUseLayoutEffect = ({children}) => {
       Scheduler.unstable_yieldValue('BrokenUseLayoutEffect render');
 
       React.useLayoutEffect(() => {
@@ -518,7 +518,7 @@ describe('ReactErrorBoundaries', () => {
         throw new Error('Hello');
       });
 
-      return props.children;
+      return children;
     };
 
     NoopErrorBoundary = class extends React.Component {

diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js
@@ -18,7 +18,12 @@ const enableSuspenseServerRenderer = require('shared/ReactFeatureFlags')
   .enableSuspenseServerRenderer;
 
 function normalizeCodeLocInfo(str) {
-  return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+  return (
+    str &&
+    str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
+      return '\n    in ' + name + ' (at **)';
+    })
+  );
 }
 
 describe('ReactDOMServer', () => {

diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js
@@ -1615,7 +1615,7 @@ describe('ReactUpdates', () => {
               Scheduler.unstable_clearYields();
             }
             expect(error).toContain('Warning: Maximum update depth exceeded.');
-            expect(stack).toContain('in NonTerminating');
+            expect(stack).toContain(' NonTerminating');
             // rethrow error to prevent going into an infinite loop when act() exits
             throw error;
           });

diff --git a/packages/react-native-renderer/src/__tests__/ReactNativeError-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactNativeError-test.internal.js
@@ -16,7 +16,12 @@ let createReactNativeComponentClass;
 let computeComponentStackForErrorReporting;
 
 function normalizeCodeLocInfo(str) {
-  return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+  return (
+    str &&
+    str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
+      return '\n    in ' + name + ' (at **)';
+    })
+  );
 }
 
 describe('ReactNativeError', () => {
@@ -74,20 +79,11 @@ describe('ReactNativeError', () => {
       computeComponentStackForErrorReporting(reactTag),
     );
 
-    if (__DEV__) {
-      expect(componentStack).toBe(
-        '\n' +
-          '    in View (at **)\n' +
-          '    in FunctionComponent (at **)\n' +
-          '    in ClassComponent (at **)',
-      );
-    } else {
-      expect(componentStack).toBe(
-        '\n' +
-          '    in View\n' +
-          '    in FunctionComponent\n' +
-          '    in ClassComponent',
-      );
-    }
+    expect(componentStack).toBe(
+      '\n' +
+        '    in View (at **)\n' +
+        '    in FunctionComponent (at **)\n' +
+        '    in ClassComponent (at **)',
+    );
   });
 });