From ac9322ea1a77b03f33ff2b047b0db831cf19fe3c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 22 Jul 2024 22:02:41 -0400 Subject: [PATCH 1/2] Prefix owner stacks added to the console.log with the current stack The current stack is available in the native UI but that's hidden by default so you don't see the actual current component on the stack. This is unlike the native async stacks UI where they're all together. So we prefix the stack with the current stack first. --- .../src/__tests__/componentStacks-test.js | 4 +- .../src/__tests__/console-test.js | 21 +++++---- .../backend/DevToolsFiberComponentStack.js | 31 ------------- .../src/backend/console.js | 45 +++++++++++++------ 4 files changed, 46 insertions(+), 55 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js index 6bbdc899b55bb..b99db5c540097 100644 --- a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js +++ b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js @@ -56,13 +56,13 @@ describe('component stack', () => { expect(mockError).toHaveBeenCalledWith( 'Test error.', - (supportsOwnerStacks ? '' : '\n in Child (at **)') + + '\n in Child (at **)' + '\n in Parent (at **)' + '\n in Grandparent (at **)', ); expect(mockWarn).toHaveBeenCalledWith( 'Test warning.', - (supportsOwnerStacks ? '' : '\n in Child (at **)') + + '\n in Child (at **)' + '\n in Parent (at **)' + '\n in Grandparent (at **)', ); diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js index 56ff240170dcd..da1990921dc29 100644 --- a/packages/react-devtools-shared/src/__tests__/console-test.js +++ b/packages/react-devtools-shared/src/__tests__/console-test.js @@ -232,7 +232,7 @@ describe('console', () => { expect(mockWarn.mock.calls[0][0]).toBe('warn'); expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); expect(mockError).toHaveBeenCalledTimes(1); @@ -240,7 +240,7 @@ describe('console', () => { expect(mockError.mock.calls[0][0]).toBe('error'); expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); }); @@ -279,7 +279,8 @@ describe('console', () => { expect(mockWarn.mock.calls[0][0]).toBe('active warn'); expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual( supportsOwnerStacks - ? '\n in Parent (at **)' + ? // TODO: It would be nice to have a Child stack frame here since it's just the effect function. + '\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); expect(mockWarn.mock.calls[1]).toHaveLength(2); @@ -497,7 +498,7 @@ describe('console', () => { expect(mockWarn.mock.calls[0][0]).toBe('warn'); expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); expect(mockError).toHaveBeenCalledTimes(1); @@ -505,7 +506,7 @@ describe('console', () => { expect(mockError.mock.calls[0][0]).toBe('error'); expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); }); @@ -1032,7 +1033,7 @@ describe('console', () => { expect(mockWarn.mock.calls[0]).toHaveLength(2); expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); expect(mockWarn.mock.calls[1]).toHaveLength(3); @@ -1042,7 +1043,8 @@ describe('console', () => { expect(mockWarn.mock.calls[1][1]).toMatch('warn'); expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][2]).trim()).toEqual( supportsOwnerStacks - ? 'in Parent (at **)' + ? 'in Object.overrideMethod (at **)' + // TODO: This leading frame is due to our extra wrapper that shouldn't exist. + '\n in Child (at **)\n in Parent (at **)' : 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); @@ -1050,7 +1052,7 @@ describe('console', () => { expect(mockError.mock.calls[0]).toHaveLength(2); expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toEqual( supportsOwnerStacks - ? '\n in Parent (at **)' + ? '\n in Child (at **)\n in Parent (at **)' : '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); expect(mockError.mock.calls[1]).toHaveLength(3); @@ -1060,7 +1062,8 @@ describe('console', () => { expect(mockError.mock.calls[1][1]).toEqual('error'); expect(normalizeCodeLocInfo(mockError.mock.calls[1][2]).trim()).toEqual( supportsOwnerStacks - ? 'in Parent (at **)' + ? 'in Object.overrideMethod (at **)' + // TODO: This leading frame is due to our extra wrapper that shouldn't exist. + '\n in Child (at **)\n in Parent (at **)' : 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)', ); }); diff --git a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js index 277bc2e520c62..9a457ada7dd20 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js @@ -121,13 +121,6 @@ export function supportsOwnerStacks(fiber: Fiber): boolean { return fiber._debugStack !== undefined; } -function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string { - // We use this because we don't actually want to describe the line of the component - // but just the component name. - const name = fn ? fn.displayName || fn.name : ''; - return name ? describeBuiltInComponentFrame(name) : ''; -} - export function getOwnerStackByFiberInDev( workTagMap: WorkTagMap, workInProgress: Fiber, @@ -140,10 +133,6 @@ export function getOwnerStackByFiberInDev( HostComponent, SuspenseComponent, SuspenseListComponent, - FunctionComponent, - SimpleMemoComponent, - ForwardRef, - ClassComponent, } = workTagMap; try { let info = ''; @@ -159,8 +148,6 @@ export function getOwnerStackByFiberInDev( // on the regular stack that's currently executing. However, for built-ins there is no such // named stack frame and it would be ignored as being internal anyway. Therefore we add // add one extra frame just to describe the "current" built-in component by name. - // Similarly, if there is no owner at all, then there's no stack frame so we add the name - // of the root component to the stack to know which component is currently executing. switch (workInProgress.tag) { case HostHoistable: case HostSingleton: @@ -173,24 +160,6 @@ export function getOwnerStackByFiberInDev( case SuspenseListComponent: info += describeBuiltInComponentFrame('SuspenseList'); break; - case FunctionComponent: - case SimpleMemoComponent: - case ClassComponent: - if (!workInProgress._debugOwner && info === '') { - // Only if we have no other data about the callsite do we add - // the component name as the single stack frame. - info += describeFunctionComponentFrameWithoutLineNumber( - workInProgress.type, - ); - } - break; - case ForwardRef: - if (!workInProgress._debugOwner && info === '') { - info += describeFunctionComponentFrameWithoutLineNumber( - workInProgress.type.render, - ); - } - break; } let owner: void | null | Fiber | ReactComponentInfo = workInProgress; diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index 2057727c55bca..e0217b348f453 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -32,6 +32,7 @@ import { supportsOwnerStacks, supportsNativeConsoleTasks, } from './DevToolsFiberComponentStack'; +import {formatOwnerStack} from './DevToolsOwnerStack'; import {castBool, castBrowserTheme} from '../utils'; const OVERRIDE_CONSOLE_METHODS = ['error', 'trace', 'warn']; @@ -252,17 +253,31 @@ export function patch({ consoleSettingsRef.appendComponentStack && !supportsNativeConsoleTasks(current) ) { - const componentStack = supportsOwnerStacks(current) - ? getOwnerStackByFiberInDev( - workTagMap, - current, - (currentDispatcherRef: any), - ) - : getStackByFiberInDevAndProd( - workTagMap, - current, - (currentDispatcherRef: any), - ); + const enableOwnerStacks = supportsOwnerStacks(current); + let componentStack = ''; + if (supportsOwnerStacks(current)) { + // 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), + ); + } 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 @@ -272,13 +287,17 @@ export function patch({ // 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 + fakeError.name = enableOwnerStacks + ? 'Stack' + : '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 + ? (enableOwnerStacks + ? 'Error Stack:' + : 'Error Component Stack:') + componentStack : componentStack; if (alreadyHasComponentStack) { // Only modify the component stack if it matches what we would've added anyway. From d69cf59c46ee90401fe3d51613eaced2071c4edc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 23 Jul 2024 15:47:22 -0400 Subject: [PATCH 2/2] Reuse variable instead of recomputing check Co-authored-by: Ruslan Lesiutin --- packages/react-devtools-shared/src/backend/console.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index e0217b348f453..dd8e9e9f27e60 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -255,7 +255,7 @@ export function patch({ ) { const enableOwnerStacks = supportsOwnerStacks(current); let componentStack = ''; - if (supportsOwnerStacks(current)) { + 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