Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DevTools] Print component stacks as error objects to get source mapping #30289

Merged
merged 2 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,7 @@ module.exports = {
'packages/react-devtools-extensions/**/*.js',
'packages/react-devtools-shared/src/hook.js',
'packages/react-devtools-shared/src/backend/console.js',
'packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js',
],
globals: {
__IS_CHROME__: 'readonly',
Expand Down
2 changes: 2 additions & 0 deletions packages/react-devtools-core/webpack.backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ module.exports = {
__PROFILE__: false,
__TEST__: NODE_ENV === 'test',
__IS_FIREFOX__: false,
__IS_CHROME__: false,
__IS_EDGE__: false,
Comment on lines 71 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably add __IS_NATIVE__ here as well.

'process.env.DEVTOOLS_PACKAGE': `"react-devtools-core"`,
'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
Expand Down
4 changes: 4 additions & 0 deletions packages/react-devtools-inline/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ module.exports = {
__EXTENSION__: false,
__PROFILE__: false,
__TEST__: NODE_ENV === 'test',
// TODO: Should this be feature tested somehow?
__IS_CHROME__: false,
__IS_FIREFOX__: false,
__IS_EDGE__: false,
Comment on lines +76 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, react-devtools-inline is shipped as an npm package, so these variables don't make much sense for it. Instead, it should be checked at runtime, not build time.

Will update this later

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure why more of these weren't reached and subsequently errored before. Not sure why just the ones I added mattered.

'process.env.DEVTOOLS_PACKAGE': `"react-devtools-inline"`,
'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
'process.env.EDITOR_URL': EDITOR_URL != null ? `"${EDITOR_URL}"` : null,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/__tests__/console-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ describe('console', () => {
);
expect(mockWarn.mock.calls[1]).toHaveLength(3);
expect(mockWarn.mock.calls[1][0]).toEqual(
'\x1b[2;38;2;124;124;124m%s %s\x1b[0m',
'\x1b[2;38;2;124;124;124m%s %o\x1b[0m',
);
expect(mockWarn.mock.calls[1][1]).toMatch('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][2]).trim()).toEqual(
Expand All @@ -1014,7 +1014,7 @@ describe('console', () => {
);
expect(mockError.mock.calls[1]).toHaveLength(3);
expect(mockError.mock.calls[1][0]).toEqual(
'\x1b[2;38;2;124;124;124m%s %s\x1b[0m',
'\x1b[2;38;2;124;124;124m%s %o\x1b[0m',
);
expect(mockError.mock.calls[1][1]).toEqual('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[1][2]).trim()).toEqual(
Expand Down
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/__tests__/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,9 @@ export function overrideFeatureFlags(overrideFlags) {
}

export function normalizeCodeLocInfo(str) {
if (typeof str === 'object' && str !== null) {
str = str.stack;
}
if (typeof str !== 'string') {
return str;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,19 @@ export function describeBuiltInComponentFrame(name: string): string {
prefix = (match && match[1]) || '';
}
}
let suffix = '';
if (__IS_CHROME__ || __IS_EDGE__) {
suffix = ' (<anonymous>)';
} else if (__IS_FIREFOX__) {
suffix = '@unknown:0:0';
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome parses the name as the location if it doesn't have one. <anonymous> is the only special case that works I think.

Firefox skips lines that don't have location so we need to add a fake one which can be anything really. I don't think there's any special case that doesn't linkify it.

Safari is fine without it.

// We use the prefix to ensure our stacks line up with native stack frames.
return '\n' + prefix + name;
// We use a suffix to ensure it gets parsed natively.
return '\n' + prefix + name + suffix;
}

export function describeDebugInfoFrame(name: string, env: ?string): string {
return describeBuiltInComponentFrame(name + (env ? ' (' + env + ')' : ''));
return describeBuiltInComponentFrame(name + (env ? ' [' + env + ']' : ''));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because parenthesis are parsed as part of the location in Chrome we need to replace those with brackets. Not sure why I used parenthesis in the first place really.

}

let reentry = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export function describeFiber(
currentDispatcherRef: CurrentDispatcherRef,
): string {
const {
HostHoistable,
HostSingleton,
HostComponent,
LazyComponent,
SuspenseComponent,
Expand All @@ -40,6 +42,8 @@ export function describeFiber(
} = workTagMap;

switch (workInProgress.tag) {
case HostHoistable:
case HostSingleton:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing previously leading to missing frames for html/body which lead to mismatches.

case HostComponent:
return describeBuiltInComponentFrame(workInProgress.type);
case LazyComponent:
Expand Down
83 changes: 62 additions & 21 deletions packages/react-devtools-shared/src/backend/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ function isStrictModeOverride(args: Array<any>): boolean {
}
}

// We add a suffix to some frames that older versions of React didn't do.
// To compare if it's equivalent we strip out the suffix to see if they're
// still equivalent. Similarly, we sometimes use [] and sometimes () so we
// strip them to for the comparison.
const frameDiffs = / \(\<anonymous\>\)$|\@unknown\:0\:0$|\(|\)|\[|\]/gm;
function areStackTracesEqual(a: string, b: string): boolean {
return a.replace(frameDiffs, '') === b.replace(frameDiffs, '');
}

function restorePotentiallyModifiedArgs(args: Array<any>): Array<any> {
// If the arguments don't have any styles applied, then just copy
if (!isStrictModeOverride(args)) {
Expand Down Expand Up @@ -204,17 +213,11 @@ export function patch({

// $FlowFixMe[missing-local-annot]
const overrideMethod = (...args) => {
let shouldAppendWarningStack = false;
if (method !== 'log') {
if (consoleSettingsRef.appendComponentStack) {
const lastArg = args.length > 0 ? args[args.length - 1] : null;
const alreadyHasComponentStack =
typeof lastArg === 'string' && isStringComponentStack(lastArg);

// If we are ever called with a string that already has a component stack,
// e.g. a React error/warning, don't append a second stack.
shouldAppendWarningStack = !alreadyHasComponentStack;
}
let alreadyHasComponentStack = false;
if (method !== 'log' && consoleSettingsRef.appendComponentStack) {
const lastArg = args.length > 0 ? args[args.length - 1] : null;
alreadyHasComponentStack =
typeof lastArg === 'string' && isStringComponentStack(lastArg); // The last argument should be a component stack.
}

const shouldShowInlineWarningsAndErrors =
Expand Down Expand Up @@ -244,7 +247,7 @@ export function patch({
}

if (
shouldAppendWarningStack &&
consoleSettingsRef.appendComponentStack &&
!supportsNativeConsoleTasks(current)
) {
const componentStack = getStackByFiberInDevAndProd(
Expand All @@ -253,17 +256,55 @@ export function patch({
(currentDispatcherRef: any),
);
if (componentStack !== '') {
if (isStrictModeOverride(args)) {
if (__IS_FIREFOX__) {
args[0] = `${args[0]} %s`;
args.push(componentStack);
} else {
args[0] =
ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK;
args.push(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:
// and similarly Safari leave an expandable slot.
fakeError.name = 'Component Stack'; // This gets printed
// In Chromium, the stack property needs to start with ^[\w.]*Error\b to trigger stack
// formatting. Otherwise it is left alone. So we prefix it. Otherwise we just override it
// to our own stack.
fakeError.stack =
__IS_CHROME__ || __IS_EDGE__
? '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.
if (isStrictModeOverride(args)) {
// We do nothing to Strict Mode overrides that already has a stack
// because we have already lost some context for how to format it
// since we've already merged the stack into the log at this point.
} else if (
areStackTracesEqual(
args[args.length - 1],
componentStack,
)
) {
const firstArg = args[0];
if (
args.length > 1 &&
typeof firstArg === 'string' &&
firstArg.endsWith('%s')
) {
args[0] = firstArg.slice(0, firstArg.length - 2); // Strip the %s param
}
args[args.length - 1] = fakeError;
}
} else {
args.push(componentStack);
args.push(fakeError);
if (isStrictModeOverride(args)) {
if (__IS_FIREFOX__) {
args[0] = `${args[0]} %o`;
} else {
args[0] =
ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK;
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,4 @@ export const PROFILER_EXPORT_VERSION = 5;
export const FIREFOX_CONSOLE_DIMMING_COLOR = 'color: rgba(124, 124, 124, 0.75)';
export const ANSI_STYLE_DIMMING_TEMPLATE = '\x1b[2;38;2;124;124;124m%s\x1b[0m';
export const ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK =
'\x1b[2;38;2;124;124;124m%s %s\x1b[0m';
'\x1b[2;38;2;124;124;124m%s %o\x1b[0m';
2 changes: 2 additions & 0 deletions scripts/flow/react-devtools.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ declare const __EXTENSION__: boolean;
declare const __TEST__: boolean;

declare const __IS_FIREFOX__: boolean;
declare const __IS_CHROME__: boolean;
declare const __IS_EDGE__: boolean;
2 changes: 2 additions & 0 deletions scripts/jest/devtools/setupEnv.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ if (!global.hasOwnProperty('localStorage')) {
global.__DEV__ = process.env.NODE_ENV !== 'production';
global.__TEST__ = true;
global.__IS_FIREFOX__ = false;
global.__IS_CHROME__ = false;
global.__IS_EDGE__ = false;

const ReactVersionTestingAgainst = process.env.REACT_VERSION || ReactVersion;

Expand Down