Remove Component Stack from React Logged Warnings and Error Reporting (…

…#30308) React transpiles some of its own `console.error` calls into a helper that appends component stacks to those calls. However, this doesn't cover user space `console.error` calls - which includes React helpers that React has moved into third parties like createClass and prop-types. The idea is that any user space component can add a warning just like React can which is why React DevTools adds them too if they don't already exist. Having them appended in both places is tricky because now you have to know whether to remove them from React's logs. Similarly it's often common for server-side frameworks to forget to cover the `console.error` logs from other sources since React DevTools isn't active there. However, it's also annoying to get component stacks clogging the terminal - depending on where the log came from. In the future `console.createTask()` will cover this use case natively and when available we don't append them at all. The new strategy relies on either: - React DevTools existing to add them to React logs as well as third parties. - `console.createTask` being supported and surfaced. - A third party framework showing the component stack either in an Error Dialog or appended to terminal output. For a third party to be able to implement this they need to be able to get the component stack. To get the component stack from within a `console.error` call you need to use the `React.captureOwnerStack()` helper which is only available in `enableOwnerStacks` flag. However, it's possible to polyfill with parent stacks using internals as a stop gap. There's a question of whether React 19 should just go out with `enableOwnerStacks` to expose this but regardless I think it's best it doesn't include component stacks from the runtime for consistency. In practice it's not really a regression though because typically either of the other options exists and error dialogs don't implement `console.error` overrides anyway yet. SSR terminals might miss them but they'd only have them in DEV warnings to begin with an a subset of React warnings. Typically those are either going to happen on the client anyway or replayed. Our tests are written to assert that component stacks work in various scenarios all over the place. To ensure that this keeps working I implement a "polyfill" that is similar to that expected a server framework might do - in `assertConsoleErrorDev` and `toErrorDev`. This PR doesn't yet change www or RN since they have their own forks of consoleWithStackDev for now.
facebook · Jul 12, 2024 · 400e822 · 400e822
1 parent 1e8efe7
commit 400e822
Show file tree

Hide file tree

Showing 12 changed files with 544 additions and 183 deletions.
diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js
diff --git a/packages/internal-test-utils/consoleMock.js b/packages/internal-test-utils/consoleMock.js
@@ -44,6 +44,34 @@ const patchConsoleMethod = (
       return;
     }
 
+    // Append Component Stacks. Simulates a framework or DevTools appending them.
+    if (
+      typeof format === 'string' &&
+      (methodName === 'error' || methodName === 'warn')
+    ) {
+      const React = require('react');
+      if (React.captureOwnerStack) {
+        // enableOwnerStacks enabled. When it's always on, we can assume this case.
+        const stack = React.captureOwnerStack();
+        if (stack) {
+          format += '%s';
+          args.push(stack);
+        }
+      } else {
+        // Otherwise we have to use internals to emulate parent stacks.
+        const ReactSharedInternals =
+          React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE ||
+          React.__SERVER_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE;
+        if (ReactSharedInternals && ReactSharedInternals.getCurrentStack) {
+          const stack = ReactSharedInternals.getCurrentStack();
+          if (stack !== '') {
+            format += '%s';
+            args.push(stack);
+          }
+        }
+      }
+    }
+
     // Capture the call stack now so we can warn about it later.
     // The call stack has helpful information for the test author.
     // Don't throw yet though b'c it might be accidentally caught and suppressed.
@@ -204,7 +232,7 @@ export function assertConsoleLogsCleared() {
     if (warnings.length > 0) {
       message += `\nconsole.warn was called without assertConsoleWarnDev:\n${diff(
         '',
-        warnings.join('\n'),
+        warnings.map(normalizeComponentStack).join('\n'),
         {
           omitAnnotationLines: true,
         },
@@ -213,7 +241,7 @@ export function assertConsoleLogsCleared() {
     if (errors.length > 0) {
       message += `\nconsole.error was called without assertConsoleErrorDev:\n${diff(
         '',
-        errors.join('\n'),
+        errors.map(normalizeComponentStack).join('\n'),
         {
           omitAnnotationLines: true,
         },
@@ -249,6 +277,19 @@ function normalizeCodeLocInfo(str) {
   });
 }
 
+function normalizeComponentStack(entry) {
+  if (
+    typeof entry[0] === 'string' &&
+    entry[0].endsWith('%s') &&
+    isLikelyAComponentStack(entry[entry.length - 1])
+  ) {
+    const clone = entry.slice(0);
+    clone[clone.length - 1] = normalizeCodeLocInfo(entry[entry.length - 1]);
+    return clone;
+  }
+  return entry;
+}
+
 const isLikelyAComponentStack = message =>
   typeof message === 'string' &&
   (message.indexOf('<component stack>') > -1 ||

diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js
@@ -1436,12 +1436,23 @@ describe('ReactFlight', () => {
 
   it('should warn in DEV a child is missing keys on server component', () => {
     function NoKey({children}) {
-      return <div key="this has a key but parent doesn't" />;
+      return ReactServer.createElement('div', {
+        key: "this has a key but parent doesn't",
+      });
     }
     expect(() => {
+      // While we're on the server we need to have the Server version active to track component stacks.
+      jest.resetModules();
+      jest.mock('react', () => ReactServer);
       const transport = ReactNoopFlightServer.render(
-        <div>{Array(6).fill(<NoKey />)}</div>,
+        ReactServer.createElement(
+          'div',
+          null,
+          Array(6).fill(ReactServer.createElement(NoKey)),
+        ),
       );
+      jest.resetModules();
+      jest.mock('react', () => React);
       ReactNoopFlightClient.read(transport);
     }).toErrorDev('Each child in a list should have a unique "key" prop.');
   });
@@ -2814,7 +2825,7 @@ describe('ReactFlight', () => {
   });
 
   // @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__
-  it('should not include component stacks in replayed logs (unless DevTools add them)', () => {
+  it('should include only one component stack in replayed logs (if DevTools or polyfill adds them)', () => {
     class MyError extends Error {
       toJSON() {
         return 123;
@@ -2839,6 +2850,9 @@ describe('ReactFlight', () => {
       return ReactServer.createElement(Bar);
     }
 
+    // While we're on the server we need to have the Server version active to track component stacks.
+    jest.resetModules();
+    jest.mock('react', () => ReactServer);
     const transport = ReactNoopFlightServer.render(
       ReactServer.createElement(App),
     );
@@ -2857,6 +2871,8 @@ describe('ReactFlight', () => {
     ]);
 
     // Replay logs on the client
+    jest.resetModules();
+    jest.mock('react', () => React);
     ReactNoopFlightClient.read(transport);
     assertConsoleErrorDev(
       [
@@ -2866,8 +2882,8 @@ describe('ReactFlight', () => {
           '  <div>Womp womp: {Error}</div>\n' +
           '                  ^^^^^^^',
       ],
-      // We should not have a stack in the replay because that should be added either by console.createTask
-      // or React DevTools on the client. Neither of which we do here.
+      // We should have a stack in the replay but we don't yet set the owner from the Flight replaying
+      // so our simulated polyfill doesn't end up getting any component stacks yet.
       {withoutStack: true},
     );
   });

diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
@@ -143,7 +143,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             expect.stringContaining('%s'),
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -208,7 +209,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -274,7 +276,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             expect.stringContaining('%s'),
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -344,7 +347,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -410,7 +414,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             expect.stringContaining('%s'),
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -478,7 +483,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {

diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js
@@ -162,7 +162,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             // Addendum by React:
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
 
@@ -239,7 +240,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -309,7 +311,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             // Addendum by React:
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
 
@@ -390,7 +393,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {
@@ -460,7 +464,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             // Addendum by React:
             expect.stringContaining('An error occurred in the <Foo> component'),
             expect.stringContaining('Consider adding an error boundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
 
@@ -540,7 +545,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
               'The above error occurred in the <Foo> component',
             ),
             expect.stringContaining('ErrorBoundary'),
-            expect.stringContaining('Foo'),
+            // The component stack is not added without the polyfill/devtools.
+            // expect.stringContaining('Foo'),
           ],
         ]);
       } else {

diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js
@@ -1857,12 +1857,14 @@ describe('ReactUpdates', () => {
       }
 
       let error = null;
-      let stack = null;
+      let ownerStack = null;
       let nativeStack = null;
       const originalConsoleError = console.error;
-      console.error = (e, s) => {
+      console.error = e => {
         error = e;
-        stack = s;
+        ownerStack = gate(flags => flags.enableOwnerStacks)
+          ? React.captureOwnerStack()
+          : null;
         nativeStack = new Error().stack;
         Scheduler.log('stop');
       };
@@ -1878,12 +1880,11 @@ describe('ReactUpdates', () => {
       expect(error).toContain('Maximum update depth exceeded');
       // The currently executing effect should be on the native stack
       expect(nativeStack).toContain('at myEffect');
-      if (!gate(flags => flags.enableOwnerStacks)) {
-        // The currently running component's name is not in the owner
-        // stack because it's just its JSX callsite.
-        expect(stack).toContain('at NonTerminating');
+      if (gate(flags => flags.enableOwnerStacks)) {
+        expect(ownerStack).toContain('at App');
+      } else {
+        expect(ownerStack).toBe(null);
       }
-      expect(stack).toContain('at App');
     });
 
     it('can have nested updates if they do not cross the limit', async () => {

diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js
@@ -91,15 +91,16 @@ describe('ReactIncrementalErrorLogging', () => {
           'Consider adding an error boundary to your tree ' +
             'to customize error handling behavior.',
         ),
-        expect.stringMatching(
-          new RegExp(
-            gate(flags => flags.enableOwnerStacks)
-              ? '\\s+(in|at) ErrorThrowingComponent'
-              : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
-                '\\s+(in|at) span(.*)\n' +
-                '\\s+(in|at) div(.*)',
-          ),
-        ),
+        // The component stack is not added without the polyfill/devtools.
+        // expect.stringMatching(
+        //  new RegExp(
+        //    gate(flags => flags.enableOwnerStacks)
+        //      ? '\\s+(in|at) ErrorThrowingComponent'
+        //      : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
+        //        '\\s+(in|at) span(.*)\n' +
+        //        '\\s+(in|at) div(.*)',
+        //  ),
+        // ),
       );
     }
   });
@@ -139,15 +140,16 @@ describe('ReactIncrementalErrorLogging', () => {
           'Consider adding an error boundary to your tree ' +
             'to customize error handling behavior.',
         ),
-        expect.stringMatching(
-          new RegExp(
-            gate(flags => flags.enableOwnerStacks)
-              ? '\\s+(in|at) ErrorThrowingComponent'
-              : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
-                '\\s+(in|at) span(.*)\n' +
-                '\\s+(in|at) div(.*)',
-          ),
-        ),
+        // The component stack is not added without the polyfill/devtools.
+        // expect.stringMatching(
+        //   new RegExp(
+        //     gate(flags => flags.enableOwnerStacks)
+        //       ? '\\s+(in|at) ErrorThrowingComponent'
+        //       : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
+        //         '\\s+(in|at) span(.*)\n' +
+        //         '\\s+(in|at) div(.*)',
+        //   ),
+        // ),
       );
     }
   });
@@ -199,16 +201,17 @@ describe('ReactIncrementalErrorLogging', () => {
           'React will try to recreate this component tree from scratch ' +
             'using the error boundary you provided, ErrorBoundary.',
         ),
-        expect.stringMatching(
-          new RegExp(
-            gate(flags => flags.enableOwnerStacks)
-              ? '\\s+(in|at) ErrorThrowingComponent'
-              : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
-                '\\s+(in|at) span(.*)\n' +
-                '\\s+(in|at) ErrorBoundary(.*)\n' +
-                '\\s+(in|at) div(.*)',
-          ),
-        ),
+        // The component stack is not added without the polyfill/devtools.
+        // expect.stringMatching(
+        //   new RegExp(
+        //     gate(flags => flags.enableOwnerStacks)
+        //       ? '\\s+(in|at) ErrorThrowingComponent'
+        //       : '\\s+(in|at) ErrorThrowingComponent (.*)\n' +
+        //         '\\s+(in|at) span(.*)\n' +
+        //         '\\s+(in|at) ErrorBoundary(.*)\n' +
+        //         '\\s+(in|at) div(.*)',
+        //   ),
+        // ),
       );
     } else {
       expect(logCapturedErrorCalls[0]).toEqual(
@@ -282,13 +285,14 @@ describe('ReactIncrementalErrorLogging', () => {
           'React will try to recreate this component tree from scratch ' +
             'using the error boundary you provided, ErrorBoundary.',
         ),
-        expect.stringMatching(
-          gate(flag => flag.enableOwnerStacks)
-            ? new RegExp('\\s+(in|at) Foo')
-            : new RegExp(
-                '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)',
-              ),
-        ),
+        // The component stack is not added without the polyfill/devtools.
+        // expect.stringMatching(
+        //   gate(flag => flag.enableOwnerStacks)
+        //     ? new RegExp('\\s+(in|at) Foo')
+        //     : new RegExp(
+        //         '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)',
+        //       ),
+        // ),
       );
     } else {
       expect(console.error).toHaveBeenCalledWith(