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.

Sign up for GitHub

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

Jump to bottom

[Flight] Prefix owner stacks added to the console.log with the current stack #30427

Merged
merged 2 commits into from
Jul 23, 2024
Merged

[Flight] Prefix owner stacks added to the console.log with the current stack #30427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac9322e
Prefix owner stacks added to the console.log with the current stack
sebmarkbage Jul 23, 2024
d69cf59
Reuse variable instead of recomputing check
sebmarkbage Jul 23, 2024
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
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/__tests__/componentStacks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 **)',
);
Expand Down
21 changes: 12 additions & 9 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,15 @@ 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);
expect(mockError.mock.calls[0]).toHaveLength(2);
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 **)',
);
});
Expand Down Expand Up @@ -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.
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 only has the useEffect(() => inner function on the stack trace which don't currently have the react-bottom-stack-frame which means they're filtered out since we don't know which frames to include.

Even if we did it wouldn't have a name. It might be nice to automatically name the effect functions based on the component/hook that they're part of. Similar to how class methods are named based on both the class instance and the method.

'\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(2);
Expand Down Expand Up @@ -497,15 +498,15 @@ 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);
expect(mockError.mock.calls[0]).toHaveLength(2);
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 **)',
);
});
Expand Down Expand Up @@ -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);
Expand All @@ -1042,15 +1043,16 @@ 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.
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This happens because the patchForStrictMode methods add an extra stack frame that we don't know we need to pop off.

We should really just get rid of that extra patching because it runs late and is therefore running after potential user space patching.

We should instead reuse the early patching that we already do for the purpose of strict mode printing. That way whatever user spaces wrappers do will be before the dimming and we don't add an extra stack frame here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should instead reuse the early patching that we already do for the purpose of strict mode printing. That way whatever user spaces wrappers do will be before the dimming and we don't add an extra stack frame here.

Yup. I am planning to work on it this week.

'\n in Child (at **)\n in Parent (at **)'
: 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);

expect(mockError).toHaveBeenCalledTimes(2);
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);
Expand All @@ -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 **)',
);
});
Expand Down
31 changes: 0 additions & 31 deletions packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -140,10 +133,6 @@ export function getOwnerStackByFiberInDev(
HostComponent,
SuspenseComponent,
SuspenseListComponent,
FunctionComponent,
SimpleMemoComponent,
ForwardRef,
ClassComponent,
} = workTagMap;
try {
let info = '';
Expand All @@ -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:
Expand All @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the actual stack usually has a frame inside the current component, we don't have to add it even if there's no owner since otherwise it'll end up show up twice. I'll do a separate PR to align this with captureOwnerStack() which should also exclude these.

}

let owner: void | null | Fiber | ReactComponentInfo = workInProgress;
Expand Down
45 changes: 32 additions & 13 deletions packages/react-devtools-shared/src/backend/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -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 (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
// 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
Expand All @@ -272,13 +287,17 @@ export function patch({
// 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
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.
Expand Down
Loading