Skip to content

Commit

Permalink
[DevTools] Improve Layering Between Console and Renderer (#30925)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sebmarkbage
sebmarkbage authored Sep 9, 2024
1 parent fa3cf50 commit 0dbacf2
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 137 deletions.
33 changes: 20 additions & 13 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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') &&
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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 **)',
);
});
});
158 changes: 56 additions & 102 deletions packages/react-devtools-shared/src/backend/console.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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'];
Expand Down Expand Up @@ -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} = {};
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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'
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
}

Expand Down
70 changes: 68 additions & 2 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -912,6 +919,7 @@ export function attach(
setErrorHandler,
setSuspenseHandler,
scheduleUpdate,
getCurrentFiber,
} = renderer;
const supportsTogglingError =
typeof setErrorHandler === 'function' &&
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/backend/flight/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function attach(
global: Object,
): RendererInterface {
patchConsoleUsingWindowValues();
registerRendererWithConsole(renderer);
registerRendererWithConsole(); // TODO: Fill in the impl

return {
cleanup() {},
Expand Down
Loading

0 comments on commit 0dbacf2

Please sign in to comment.