From 491a4eacce69fec4144725beaac7141da269e8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 8 Jul 2024 18:42:58 -0400 Subject: [PATCH] [DevTools] Print component stacks as error objects to get source mapping (#30289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: Screenshot 2024-07-04 at 3 20 34 PM After: Screenshot 2024-07-05 at 6 08 28 PM Firefox: Screenshot 2024-07-05 at 6 09 50 PM The first log doesn't get a stack because it's logged before DevTools boots up and connects which is unfortunate. The second log already has a stack printed by React (this is on stable) it gets replaced by our object now. The third and following logs don't have a stack and get one appended. I only turn the stack into an error object if it matches what we would emit from DevTools anyway. Otherwise we assume it's not React. Since I had to change the format slightly to make this work, I first normalize the stack slightly before doing a comparison since it won't be 1:1. --- .eslintrc.js | 1 + .../react-devtools-core/webpack.backend.js | 2 + .../react-devtools-inline/webpack.config.js | 4 + .../src/__tests__/console-test.js | 4 +- .../src/__tests__/utils.js | 3 + .../backend/DevToolsComponentStackFrame.js | 11 ++- .../backend/DevToolsFiberComponentStack.js | 4 + .../src/backend/console.js | 83 ++++++++++++++----- .../react-devtools-shared/src/constants.js | 2 +- scripts/flow/react-devtools.js | 2 + scripts/jest/devtools/setupEnv.js | 2 + 11 files changed, 92 insertions(+), 26 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 1d45d68055..f39437a7d1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -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', diff --git a/packages/react-devtools-core/webpack.backend.js b/packages/react-devtools-core/webpack.backend.js index 24e3ced0d7..7efd5b0b5b 100644 --- a/packages/react-devtools-core/webpack.backend.js +++ b/packages/react-devtools-core/webpack.backend.js @@ -69,6 +69,8 @@ module.exports = { __PROFILE__: false, __TEST__: NODE_ENV === 'test', __IS_FIREFOX__: false, + __IS_CHROME__: false, + __IS_EDGE__: false, 'process.env.DEVTOOLS_PACKAGE': `"react-devtools-core"`, 'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`, 'process.env.GITHUB_URL': `"${GITHUB_URL}"`, diff --git a/packages/react-devtools-inline/webpack.config.js b/packages/react-devtools-inline/webpack.config.js index 2ab8db739a..7b153bbc13 100644 --- a/packages/react-devtools-inline/webpack.config.js +++ b/packages/react-devtools-inline/webpack.config.js @@ -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, 'process.env.DEVTOOLS_PACKAGE': `"react-devtools-inline"`, 'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`, 'process.env.EDITOR_URL': EDITOR_URL != null ? `"${EDITOR_URL}"` : null, diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js index c79850901a..75e7dc1c87 100644 --- a/packages/react-devtools-shared/src/__tests__/console-test.js +++ b/packages/react-devtools-shared/src/__tests__/console-test.js @@ -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( @@ -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( diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index c0c472e681..4a42cf2703 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -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; } diff --git a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js index e20fb85e3c..e42073cc02 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js +++ b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js @@ -29,12 +29,19 @@ export function describeBuiltInComponentFrame(name: string): string { prefix = (match && match[1]) || ''; } } + let suffix = ''; + if (__IS_CHROME__ || __IS_EDGE__) { + suffix = ' ()'; + } else if (__IS_FIREFOX__) { + suffix = '@unknown:0:0'; + } // 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 + ']' : '')); } let reentry = false; diff --git a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js index a4311797de..73887c16d8 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js @@ -28,6 +28,8 @@ export function describeFiber( currentDispatcherRef: CurrentDispatcherRef, ): string { const { + HostHoistable, + HostSingleton, HostComponent, LazyComponent, SuspenseComponent, @@ -40,6 +42,8 @@ export function describeFiber( } = workTagMap; switch (workInProgress.tag) { + case HostHoistable: + case HostSingleton: case HostComponent: return describeBuiltInComponentFrame(workInProgress.type); case LazyComponent: diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index 3aedfa6e39..227298da2b 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -63,6 +63,15 @@ function isStrictModeOverride(args: Array): 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 = / \(\\)$|\@unknown\:0\:0$|\(|\)|\[|\]/gm; +function areStackTracesEqual(a: string, b: string): boolean { + return a.replace(frameDiffs, '') === b.replace(frameDiffs, ''); +} + function restorePotentiallyModifiedArgs(args: Array): Array { // If the arguments don't have any styles applied, then just copy if (!isStrictModeOverride(args)) { @@ -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 = @@ -244,7 +247,7 @@ export function patch({ } if ( - shouldAppendWarningStack && + consoleSettingsRef.appendComponentStack && !supportsNativeConsoleTasks(current) ) { const componentStack = getStackByFiberInDevAndProd( @@ -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 : + // 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; + } + } } } } diff --git a/packages/react-devtools-shared/src/constants.js b/packages/react-devtools-shared/src/constants.js index 303ae50288..52d39f2d90 100644 --- a/packages/react-devtools-shared/src/constants.js +++ b/packages/react-devtools-shared/src/constants.js @@ -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'; diff --git a/scripts/flow/react-devtools.js b/scripts/flow/react-devtools.js index 67355481e9..2b2d6b38be 100644 --- a/scripts/flow/react-devtools.js +++ b/scripts/flow/react-devtools.js @@ -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; diff --git a/scripts/jest/devtools/setupEnv.js b/scripts/jest/devtools/setupEnv.js index 019cf40e43..1021fbb587 100644 --- a/scripts/jest/devtools/setupEnv.js +++ b/scripts/jest/devtools/setupEnv.js @@ -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;