From 5c7871b2b034b91b0edf936ff6bc65ba34bd7134 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 5 Jul 2024 14:51:28 -0400 Subject: [PATCH 1/2] Print component stacks as error objects to get source mapping --- .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 | 88 ++++++++++++++----- .../react-devtools-shared/src/constants.js | 2 +- scripts/flow/react-devtools.js | 2 + scripts/jest/devtools/setupEnv.js | 2 + 11 files changed, 97 insertions(+), 26 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 1d45d68055078..f39437a7d143e 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 24e3ced0d7b46..7efd5b0b5be15 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 2ab8db739a599..7b153bbc13b13 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 c79850901a825..75e7dc1c87b84 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 c0c472e6816c2..4a42cf2703ffb 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 e20fb85e3cf5f..e42073cc02ee5 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 a4311797de36c..73887c16d8584 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 3aedfa6e39535..f5848c2e713d3 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,60 @@ 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 ( + areStackTracesEqual( + args[args.length - 1], + componentStack, + ) + ) { + args[args.length - 1] = fakeError; + if (isStrictModeOverride(args)) { + if (__IS_FIREFOX__) { + args[0] = `${args[0]} %o`; + } else { + args[0] = + ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK; + } + } else { + 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 + } + } } } 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 303ae502880a8..52d39f2d90b07 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 67355481e9fd4..2b2d6b38bec48 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 019cf40e43d83..1021fbb587c54 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; From 5b3ebf021365f18cb9024550c45aadcfb8c87a28 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 8 Jul 2024 18:32:42 -0400 Subject: [PATCH 2/2] Don't try to format if the log already had a component stack in strict mode override We've already merged the stack with the message at this point so would need further refactoring to handle. This is non-essential and often just fully hidden. --- .../src/backend/console.js | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index f5848c2e713d3..227298da2bbbe 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -275,30 +275,25 @@ export function patch({ 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 ( + 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, ) ) { - args[args.length - 1] = fakeError; - if (isStrictModeOverride(args)) { - if (__IS_FIREFOX__) { - args[0] = `${args[0]} %o`; - } else { - args[0] = - ANSI_STYLE_DIMMING_TEMPLATE_WITH_COMPONENT_STACK; - } - } else { - 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 - } + 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(fakeError);