Skip to content

Commit e2cac67

Browse files
sebmarkbagehoxyq
sebmarkbage
and
hoxyq
authored
[Flight] Prefix owner stacks added to the console.log with the current stack (#30427)
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. <img width="279" alt="Screenshot 2024-07-22 at 10 05 13 PM" src="https://github.com/user-attachments/assets/8f568fda-6493-416d-a0be-661caf44d808"> --------- Co-authored-by: Ruslan Lesiutin <[email protected]>
1 parent 942eb80 commit e2cac67

File tree

4 files changed

+46
-55
lines changed

4 files changed

+46
-55
lines changed

‎packages/react-devtools-shared/src/__tests__/componentStacks-test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ describe('component stack', () => {
5656

5757
expect(mockError).toHaveBeenCalledWith(
5858
'Test error.',
59-
(supportsOwnerStacks ? '' : '\n in Child (at **)') +
59+
'\n in Child (at **)' +
6060
'\n in Parent (at **)' +
6161
'\n in Grandparent (at **)',
6262
);
6363
expect(mockWarn).toHaveBeenCalledWith(
6464
'Test warning.',
65-
(supportsOwnerStacks ? '' : '\n in Child (at **)') +
65+
'\n in Child (at **)' +
6666
'\n in Parent (at **)' +
6767
'\n in Grandparent (at **)',
6868
);

‎packages/react-devtools-shared/src/__tests__/console-test.js‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -232,15 +232,15 @@ describe('console', () => {
232232
expect(mockWarn.mock.calls[0][0]).toBe('warn');
233233
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
234234
supportsOwnerStacks
235-
? '\n in Parent (at **)'
235+
? '\n in Child (at **)\n in Parent (at **)'
236236
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
237237
);
238238
expect(mockError).toHaveBeenCalledTimes(1);
239239
expect(mockError.mock.calls[0]).toHaveLength(2);
240240
expect(mockError.mock.calls[0][0]).toBe('error');
241241
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
242242
supportsOwnerStacks
243-
? '\n in Parent (at **)'
243+
? '\n in Child (at **)\n in Parent (at **)'
244244
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
245245
);
246246
});
@@ -279,7 +279,8 @@ describe('console', () => {
279279
expect(mockWarn.mock.calls[0][0]).toBe('active warn');
280280
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
281281
supportsOwnerStacks
282-
? '\n in Parent (at **)'
282+
? // TODO: It would be nice to have a Child stack frame here since it's just the effect function.
283+
'\n in Parent (at **)'
283284
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
284285
);
285286
expect(mockWarn.mock.calls[1]).toHaveLength(2);
@@ -497,15 +498,15 @@ describe('console', () => {
497498
expect(mockWarn.mock.calls[0][0]).toBe('warn');
498499
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
499500
supportsOwnerStacks
500-
? '\n in Parent (at **)'
501+
? '\n in Child (at **)\n in Parent (at **)'
501502
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
502503
);
503504
expect(mockError).toHaveBeenCalledTimes(1);
504505
expect(mockError.mock.calls[0]).toHaveLength(2);
505506
expect(mockError.mock.calls[0][0]).toBe('error');
506507
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
507508
supportsOwnerStacks
508-
? '\n in Parent (at **)'
509+
? '\n in Child (at **)\n in Parent (at **)'
509510
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
510511
);
511512
});
@@ -1032,7 +1033,7 @@ describe('console', () => {
10321033
expect(mockWarn.mock.calls[0]).toHaveLength(2);
10331034
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
10341035
supportsOwnerStacks
1035-
? '\n in Parent (at **)'
1036+
? '\n in Child (at **)\n in Parent (at **)'
10361037
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
10371038
);
10381039
expect(mockWarn.mock.calls[1]).toHaveLength(3);
@@ -1042,15 +1043,16 @@ describe('console', () => {
10421043
expect(mockWarn.mock.calls[1][1]).toMatch('warn');
10431044
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][2]).trim()).toEqual(
10441045
supportsOwnerStacks
1045-
? 'in Parent (at **)'
1046+
? 'in Object.overrideMethod (at **)' + // TODO: This leading frame is due to our extra wrapper that shouldn't exist.
1047+
'\n in Child (at **)\n in Parent (at **)'
10461048
: 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
10471049
);
10481050

10491051
expect(mockError).toHaveBeenCalledTimes(2);
10501052
expect(mockError.mock.calls[0]).toHaveLength(2);
10511053
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toEqual(
10521054
supportsOwnerStacks
1053-
? '\n in Parent (at **)'
1055+
? '\n in Child (at **)\n in Parent (at **)'
10541056
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
10551057
);
10561058
expect(mockError.mock.calls[1]).toHaveLength(3);
@@ -1060,7 +1062,8 @@ describe('console', () => {
10601062
expect(mockError.mock.calls[1][1]).toEqual('error');
10611063
expect(normalizeCodeLocInfo(mockError.mock.calls[1][2]).trim()).toEqual(
10621064
supportsOwnerStacks
1063-
? 'in Parent (at **)'
1065+
? 'in Object.overrideMethod (at **)' + // TODO: This leading frame is due to our extra wrapper that shouldn't exist.
1066+
'\n in Child (at **)\n in Parent (at **)'
10641067
: 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
10651068
);
10661069
});

‎packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js‎

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,6 @@ export function supportsOwnerStacks(fiber: Fiber): boolean {
121121
return fiber._debugStack !== undefined;
122122
}
123123

124-
function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string {
125-
// We use this because we don't actually want to describe the line of the component
126-
// but just the component name.
127-
const name = fn ? fn.displayName || fn.name : '';
128-
return name ? describeBuiltInComponentFrame(name) : '';
129-
}
130-
131124
export function getOwnerStackByFiberInDev(
132125
workTagMap: WorkTagMap,
133126
workInProgress: Fiber,
@@ -140,10 +133,6 @@ export function getOwnerStackByFiberInDev(
140133
HostComponent,
141134
SuspenseComponent,
142135
SuspenseListComponent,
143-
FunctionComponent,
144-
SimpleMemoComponent,
145-
ForwardRef,
146-
ClassComponent,
147136
} = workTagMap;
148137
try {
149138
let info = '';
@@ -159,8 +148,6 @@ export function getOwnerStackByFiberInDev(
159148
// on the regular stack that's currently executing. However, for built-ins there is no such
160149
// named stack frame and it would be ignored as being internal anyway. Therefore we add
161150
// add one extra frame just to describe the "current" built-in component by name.
162-
// Similarly, if there is no owner at all, then there's no stack frame so we add the name
163-
// of the root component to the stack to know which component is currently executing.
164151
switch (workInProgress.tag) {
165152
case HostHoistable:
166153
case HostSingleton:
@@ -173,24 +160,6 @@ export function getOwnerStackByFiberInDev(
173160
case SuspenseListComponent:
174161
info += describeBuiltInComponentFrame('SuspenseList');
175162
break;
176-
case FunctionComponent:
177-
case SimpleMemoComponent:
178-
case ClassComponent:
179-
if (!workInProgress._debugOwner && info === '') {
180-
// Only if we have no other data about the callsite do we add
181-
// the component name as the single stack frame.
182-
info += describeFunctionComponentFrameWithoutLineNumber(
183-
workInProgress.type,
184-
);
185-
}
186-
break;
187-
case ForwardRef:
188-
if (!workInProgress._debugOwner && info === '') {
189-
info += describeFunctionComponentFrameWithoutLineNumber(
190-
workInProgress.type.render,
191-
);
192-
}
193-
break;
194163
}
195164

196165
let owner: void | null | Fiber | ReactComponentInfo = workInProgress;

‎packages/react-devtools-shared/src/backend/console.js‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import {
3232
supportsOwnerStacks,
3333
supportsNativeConsoleTasks,
3434
} from './DevToolsFiberComponentStack';
35+
import {formatOwnerStack} from './DevToolsOwnerStack';
3536
import {castBool, castBrowserTheme} from '../utils';
3637

3738
const OVERRIDE_CONSOLE_METHODS = ['error', 'trace', 'warn'];
@@ -252,17 +253,31 @@ export function patch({
252253
consoleSettingsRef.appendComponentStack &&
253254
!supportsNativeConsoleTasks(current)
254255
) {
255-
const componentStack = supportsOwnerStacks(current)
256-
? getOwnerStackByFiberInDev(
257-
workTagMap,
258-
current,
259-
(currentDispatcherRef: any),
260-
)
261-
: getStackByFiberInDevAndProd(
262-
workTagMap,
263-
current,
264-
(currentDispatcherRef: any),
265-
);
256+
const enableOwnerStacks = supportsOwnerStacks(current);
257+
let componentStack = '';
258+
if (enableOwnerStacks) {
259+
// Prefix the owner stack with the current stack. I.e. what called
260+
// console.error. While this will also be part of the native stack,
261+
// it is hidden and not presented alongside this argument so we print
262+
// them all together.
263+
const topStackFrames = formatOwnerStack(
264+
new Error('react-stack-top-frame'),
265+
);
266+
if (topStackFrames) {
267+
componentStack += '\n' + topStackFrames;
268+
}
269+
componentStack += getOwnerStackByFiberInDev(
270+
workTagMap,
271+
current,
272+
(currentDispatcherRef: any),
273+
);
274+
} else {
275+
componentStack = getStackByFiberInDevAndProd(
276+
workTagMap,
277+
current,
278+
(currentDispatcherRef: any),
279+
);
280+
}
266281
if (componentStack !== '') {
267282
// Create a fake Error so that when we print it we get native source maps. Every
268283
// browser will print the .stack property of the error and then parse it back for source
@@ -272,13 +287,17 @@ export function patch({
272287
// In Chromium, only the stack property is printed but in Firefox the <name>:<message>
273288
// gets printed so to make the colon make sense, we name it so we print Component Stack:
274289
// and similarly Safari leave an expandable slot.
275-
fakeError.name = 'Component Stack'; // This gets printed
290+
fakeError.name = enableOwnerStacks
291+
? 'Stack'
292+
: 'Component Stack'; // This gets printed
276293
// In Chromium, the stack property needs to start with ^[\w.]*Error\b to trigger stack
277294
// formatting. Otherwise it is left alone. So we prefix it. Otherwise we just override it
278295
// to our own stack.
279296
fakeError.stack =
280297
__IS_CHROME__ || __IS_EDGE__
281-
? 'Error Component Stack:' + componentStack
298+
? (enableOwnerStacks
299+
? 'Error Stack:'
300+
: 'Error Component Stack:') + componentStack
282301
: componentStack;
283302
if (alreadyHasComponentStack) {
284303
// Only modify the component stack if it matches what we would've added anyway.

0 commit comments

Comments
 (0)