[DevTools] Improve Layering Between Console and Renderer

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() {},