[DevTools] Improve Layering Between Console and Renderer (#30925)

The console instrumentation should not know about things like Fibers.
Only the renderer bindings should know about that stuff. We can improve
the layering by just moving all that stuff behind a `getComponentStack`
helper that gets injected by the renderer.

This sets us up for the Flight renderer #30906 to have its own
implementation of this function.

Author: sebmarkbage

packages/react-devtools-shared/src/__tests__/console-test.js

@@ -54,14 +54,6 @@ describe('console', () => {
       fakeConsole,
     );
 
-    const inject = global.__REACT_DEVTOOLS_GLOBAL_HOOK__.inject;
-    global.__REACT_DEVTOOLS_GLOBAL_HOOK__.inject = internals => {
-      rendererID = inject(internals);
-
-      Console.registerRenderer(internals);
-      return rendererID;
-    };
-
     React = require('react');
     if (
       React.version.startsWith('19') &&
@@ -1100,9 +1092,17 @@ describe('console error', () => {
     global.__REACT_DEVTOOLS_GLOBAL_HOOK__.inject = internals => {
       inject(internals);
 
-      Console.registerRenderer(internals, () => {
-        throw Error('foo');
-      });
+      Console.registerRenderer(
+        () => {
+          throw Error('foo');
+        },
+        () => {
+          return {
+            enableOwnerStacks: true,
+            componentStack: '\n    at FakeStack (fake-file)',
+          };
+        },
+      );
     };
 
     React = require('react');
@@ -1142,11 +1142,18 @@ describe('console error', () => {
     expect(mockLog.mock.calls[0][0]).toBe('log');
 
     expect(mockWarn).toHaveBeenCalledTimes(1);
-    expect(mockWarn.mock.calls[0]).toHaveLength(1);
+    expect(mockWarn.mock.calls[0]).toHaveLength(2);
     expect(mockWarn.mock.calls[0][0]).toBe('warn');
+    // An error in showInlineWarningsAndErrors doesn't need to break component stacks.
+    expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
+      '\n    in FakeStack (at **)',
+    );
 
     expect(mockError).toHaveBeenCalledTimes(1);
-    expect(mockError.mock.calls[0]).toHaveLength(1);
+    expect(mockError.mock.calls[0]).toHaveLength(2);
     expect(mockError.mock.calls[0][0]).toBe('error');
+    expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
+      '\n    in FakeStack (at **)',
+    );
   });
 });

packages/react-devtools-shared/src/backend/console.js

@@ -7,14 +7,7 @@
  * @flow
  */
 
-import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';
-import type {
-  LegacyDispatcherRef,
-  CurrentDispatcherRef,
-  ReactRenderer,
-  WorkTagMap,
-  ConsolePatchSettings,
-} from './types';
+import type {ConsolePatchSettings} from './types';
 
 import {
   formatConsoleArguments,
@@ -25,14 +18,6 @@ import {
   ANSI_STYLE_DIMMING_TEMPLATE,
   ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK,
 } from 'react-devtools-shared/src/constants';
-import {getInternalReactConstants, getDispatcherRef} from './fiber/renderer';
-import {
-  getStackByFiberInDevAndProd,
-  getOwnerStackByFiberInDev,
-  supportsOwnerStacks,
-  supportsConsoleTasks,
-} from './fiber/DevToolsFiberComponentStack';
-import {formatOwnerStack} from './shared/DevToolsOwnerStack';
 import {castBool, castBrowserTheme} from '../utils';
 
 const OVERRIDE_CONSOLE_METHODS = ['error', 'trace', 'warn'];
@@ -90,21 +75,15 @@ function restorePotentiallyModifiedArgs(args: Array<any>): Array<any> {
   }
 }
 
-type OnErrorOrWarning = (
-  fiber: Fiber,
-  type: 'error' | 'warn',
-  args: Array<any>,
-) => void;
-
-const injectedRenderers: Map<
-  ReactRenderer,
-  {
-    currentDispatcherRef: LegacyDispatcherRef | CurrentDispatcherRef,
-    getCurrentFiber: () => Fiber | null,
-    onErrorOrWarning: ?OnErrorOrWarning,
-    workTagMap: WorkTagMap,
-  },
-> = new Map();
+type OnErrorOrWarning = (type: 'error' | 'warn', args: Array<any>) => void;
+type GetComponentStack = (
+  topFrame: Error,
+) => null | {enableOwnerStacks: boolean, componentStack: string};
+
+const injectedRenderers: Array<{
+  onErrorOrWarning: ?OnErrorOrWarning,
+  getComponentStack: ?GetComponentStack,
+}> = [];
 
 let targetConsole: Object = console;
 let targetConsoleMethods: {[string]: $FlowFixMe} = {};
@@ -132,23 +111,13 @@ export function dangerous_setTargetConsoleForTesting(
 // These internals will be used if the console is patched.
 // Injecting them separately allows the console to easily be patched or un-patched later (at runtime).
 export function registerRenderer(
-  renderer: ReactRenderer,
   onErrorOrWarning?: OnErrorOrWarning,
+  getComponentStack?: GetComponentStack,
 ): void {
-  const {currentDispatcherRef, getCurrentFiber, version} = renderer;
-
-  // currentDispatcherRef gets injected for v16.8+ to support hooks inspection.
-  // getCurrentFiber gets injected for v16.9+.
-  if (currentDispatcherRef != null && typeof getCurrentFiber === 'function') {
-    const {ReactTypeOfWork} = getInternalReactConstants(version);
-
-    injectedRenderers.set(renderer, {
-      currentDispatcherRef,
-      getCurrentFiber,
-      workTagMap: ReactTypeOfWork,
-      onErrorOrWarning,
-    });
-  }
+  injectedRenderers.push({
+    onErrorOrWarning,
+    getComponentStack,
+  });
 }
 
 const consoleSettingsRef: ConsolePatchSettings = {
@@ -219,63 +188,47 @@ export function patch({
 
           // Search for the first renderer that has a current Fiber.
           // We don't handle the edge case of stacks for more than one (e.g. interleaved renderers?)
-          // eslint-disable-next-line no-for-of-loops/no-for-of-loops
-          for (const renderer of injectedRenderers.values()) {
-            const currentDispatcherRef = getDispatcherRef(renderer);
-            const {getCurrentFiber, onErrorOrWarning, workTagMap} = renderer;
-            const current: ?Fiber = getCurrentFiber();
-            if (current != null) {
-              try {
-                if (shouldShowInlineWarningsAndErrors) {
-                  // patch() is called by two places: (1) the hook and (2) the renderer backend.
-                  // The backend is what implements a message queue, so it's the only one that injects onErrorOrWarning.
-                  if (typeof onErrorOrWarning === 'function') {
-                    onErrorOrWarning(
-                      current,
-                      ((method: any): 'error' | 'warn'),
-                      // Restore and copy args before we mutate them (e.g. adding the component stack)
-                      restorePotentiallyModifiedArgs(args),
-                    );
-                  }
+          for (let i = 0; i < injectedRenderers.length; i++) {
+            const renderer = injectedRenderers[i];
+            const {getComponentStack, onErrorOrWarning} = renderer;
+            try {
+              if (shouldShowInlineWarningsAndErrors) {
+                // patch() is called by two places: (1) the hook and (2) the renderer backend.
+                // The backend is what implements a message queue, so it's the only one that injects onErrorOrWarning.
+                if (onErrorOrWarning != null) {
+                  onErrorOrWarning(
+                    ((method: any): 'error' | 'warn'),
+                    // Restore and copy args before we mutate them (e.g. adding the component stack)
+                    restorePotentiallyModifiedArgs(args),
+                  );
                 }
-
-                if (
-                  consoleSettingsRef.appendComponentStack &&
-                  !supportsConsoleTasks(current)
-                ) {
-                  const enableOwnerStacks = supportsOwnerStacks(current);
-                  let componentStack = '';
-                  if (enableOwnerStacks) {
-                    // Prefix the owner stack with the current stack. I.e. what called
-                    // console.error. While this will also be part of the native stack,
-                    // it is hidden and not presented alongside this argument so we print
-                    // them all together.
-                    const topStackFrames = formatOwnerStack(
-                      new Error('react-stack-top-frame'),
-                    );
-                    if (topStackFrames) {
-                      componentStack += '\n' + topStackFrames;
-                    }
-                    componentStack += getOwnerStackByFiberInDev(
-                      workTagMap,
-                      current,
-                      (currentDispatcherRef: any),
-                    );
-                  } else {
-                    componentStack = getStackByFiberInDevAndProd(
-                      workTagMap,
-                      current,
-                      (currentDispatcherRef: any),
-                    );
-                  }
+              }
+            } catch (error) {
+              // Don't let a DevTools or React internal error interfere with logging.
+              setTimeout(() => {
+                throw error;
+              }, 0);
+            }
+            try {
+              if (
+                consoleSettingsRef.appendComponentStack &&
+                getComponentStack != null
+              ) {
+                // This needs to be directly in the wrapper so we can pop exactly one frame.
+                const topFrame = Error('react-stack-top-frame');
+                const match = getComponentStack(topFrame);
+                if (match !== null) {
+                  const {enableOwnerStacks, componentStack} = match;
+                  // Empty string means we have a match but no component stack.
+                  // We don't need to look in other renderers but we also don't add anything.
                   if (componentStack !== '') {
                     // Create a fake Error so that when we print it we get native source maps. Every
                     // browser will print the .stack property of the error and then parse it back for source
                     // mapping. Rather than print the internal slot. So it doesn't matter that the internal
                     // slot doesn't line up.
                     const fakeError = new Error('');
                     // In Chromium, only the stack property is printed but in Firefox the <name>:<message>
-                    // gets printed so to make the colon make sense, we name it so we print Component Stack:
+                    // gets printed so to make the colon make sense, we name it so we print Stack:
                     // and similarly Safari leave an expandable slot.
                     fakeError.name = enableOwnerStacks
                       ? 'Stack'
@@ -289,6 +242,7 @@ export function patch({
                             ? 'Error Stack:'
                             : 'Error Component Stack:') + componentStack
                         : componentStack;
+
                     if (alreadyHasComponentStack) {
                       // Only modify the component stack if it matches what we would've added anyway.
                       // Otherwise we assume it was a non-React stack.
@@ -324,15 +278,15 @@ export function patch({
                       }
                     }
                   }
+                  // Don't add stacks from other renderers.
+                  break;
                 }
-              } catch (error) {
-                // Don't let a DevTools or React internal error interfere with logging.
-                setTimeout(() => {
-                  throw error;
-                }, 0);
-              } finally {
-                break;
               }
+            } catch (error) {
+              // Don't let a DevTools or React internal error interfere with logging.
+              setTimeout(() => {
+                throw error;
+              }, 0);
             }
           }
 

packages/react-devtools-shared/src/backend/fiber/renderer.js

@@ -106,6 +106,13 @@ import {componentInfoToComponentLogsMap} from '../shared/DevToolsServerComponent
 import is from 'shared/objectIs';
 import hasOwnProperty from 'shared/hasOwnProperty';
 
+import {
+  getStackByFiberInDevAndProd,
+  getOwnerStackByFiberInDev,
+  supportsOwnerStacks,
+  supportsConsoleTasks,
+} from './DevToolsFiberComponentStack';
+
 // $FlowFixMe[method-unbinding]
 const toString = Object.prototype.toString;
 
@@ -912,6 +919,7 @@ export function attach(
     setErrorHandler,
     setSuspenseHandler,
     scheduleUpdate,
+    getCurrentFiber,
   } = renderer;
   const supportsTogglingError =
     typeof setErrorHandler === 'function' &&
@@ -1067,12 +1075,70 @@ export function attach(
     }
   }
 
+  function getComponentStack(
+    topFrame: Error,
+  ): null | {enableOwnerStacks: boolean, componentStack: string} {
+    if (getCurrentFiber === undefined) {
+      // Expected this to be part of the renderer. Ignore.
+      return null;
+    }
+    const current = getCurrentFiber();
+    if (current === null) {
+      // Outside of our render scope.
+      return null;
+    }
+
+    if (supportsConsoleTasks(current)) {
+      // This will be handled natively by console.createTask. No need for
+      // DevTools to add it.
+      return null;
+    }
+
+    const dispatcherRef = getDispatcherRef(renderer);
+    if (dispatcherRef === undefined) {
+      return null;
+    }
+
+    const enableOwnerStacks = supportsOwnerStacks(current);
+    let componentStack = '';
+    if (enableOwnerStacks) {
+      // Prefix the owner stack with the current stack. I.e. what called
+      // console.error. While this will also be part of the native stack,
+      // it is hidden and not presented alongside this argument so we print
+      // them all together.
+      const topStackFrames = formatOwnerStack(topFrame);
+      if (topStackFrames) {
+        componentStack += '\n' + topStackFrames;
+      }
+      componentStack += getOwnerStackByFiberInDev(
+        ReactTypeOfWork,
+        current,
+        dispatcherRef,
+      );
+    } else {
+      componentStack = getStackByFiberInDevAndProd(
+        ReactTypeOfWork,
+        current,
+        dispatcherRef,
+      );
+    }
+    return {enableOwnerStacks, componentStack};
+  }
+
   // Called when an error or warning is logged during render, commit, or passive (including unmount functions).
   function onErrorOrWarning(
-    fiber: Fiber,
     type: 'error' | 'warn',
     args: $ReadOnlyArray<any>,
   ): void {
+    if (getCurrentFiber === undefined) {
+      // Expected this to be part of the renderer. Ignore.
+      return;
+    }
+    const fiber = getCurrentFiber();
+    if (fiber === null) {
+      // Outside of our render scope.
+      return;
+    }
     if (type === 'error') {
       // if this is an error simulated by us to trigger error boundary, ignore
       if (
@@ -1135,7 +1201,7 @@ export function attach(
   // Patching the console enables DevTools to do a few useful things:
   // * Append component stacks to warnings and error messages
   // * Disable logging during re-renders to inspect hooks (see inspectHooksOfFiber)
-  registerRendererWithConsole(renderer, onErrorOrWarning);
+  registerRendererWithConsole(onErrorOrWarning, getComponentStack);
 
   // The renderer interface can't read these preferences directly,
   // because it is stored in localStorage within the context of the extension.

packages/react-devtools-shared/src/backend/flight/renderer.js

@@ -21,7 +21,7 @@ export function attach(
   global: Object,
 ): RendererInterface {
   patchConsoleUsingWindowValues();
-  registerRendererWithConsole(renderer);
+  registerRendererWithConsole(); // TODO: Fill in the impl
 
   return {
     cleanup() {},