[Fiber] Prefix owner stacks with the current stack at the console call (#29697)
This information is available in the regular stack but since that's hidden behind an expando and our appended stack to logs is not hidden, it hides the most important frames like the name of the current component. This is closer to what happens to the native stack. We only include stacks if they're within a ReactFiberCallUserSpace call frame. This should be most that have a current fiber but this is critical to filtering out most React frames if the regular node_modules filter doesn't work. Most React warnings fire during the rendering phase and not inside a user space function but some do like hooks warnings and setState in render. This feature is more important if we port this to React DevTools appending stacks to all logs where it's likely to originate from inside a component and you want the line within that component to immediately part of the visible stack. One thing that kind sucks is that we don't have a reliable way to exclude React internal stack frames. We filter node_modules but it might not match. For other cases I try hard to only track the stack frame at the root of React (e.g. immediately inside createElement) until the ReactFiberCallUserSpace so we don't need the filtering to work. In this case it's hard to achieve the same thing though. This is easier in RDT because we have the start/end line and parsing of stack traces so we can use that to exclude internals but that's a lot of code/complexity for shipping within the library. For example in Safari: <img width="590" alt="Screenshot 2024-05-31 at 6 15 27 PM" src="https://github.com/facebook/react/assets/63648/2820c8c0-8a03-42e9-8678-8348f66b051a"> Ideally warnOnUseFormStateInDev and useFormState wouldn't be included since they're React internals. Before this change, the Counter.js line also wasn't included though which points to exactly where the error is within the user code. (Note Server Components have V8 formatted lines and Client Components have JSC formatted lines.)
Sebastian Markbåge committed
Jun 3, 2024 at 12:26 UTC
4dcdf21325028d7ae9bb3c2172dbbe9647a744ac
10 files changed
+64
-42
packages/react-reconciler/src/ReactCurrentFiber.js
+2
-2
@@ -44,7 +44,7 @@ export function getCurrentParentStackInDev(): string {
44
return '';
45
}
46
47
-function getCurrentFiberStackInDev(): string {
47
+function getCurrentFiberStackInDev(stack: Error): string {
48
if (__DEV__) {
49
if (current === null) {
50
return '';
@@ -54,7 +54,7 @@ function getCurrentFiberStackInDev(): string {
54
// TODO: The above comment is not actually true. We might be
55
// in a commit phase or preemptive set state callback.
56
if (enableOwnerStacks) {
57
- return getOwnerStackByFiberInDev(current);
57
+ return getOwnerStackByFiberInDev(current, stack);
58
}
59
return getStackByFiberInDevAndProd(current);
60
}
packages/react-reconciler/src/ReactFiberCallUserSpace.js
+15
-7
@@ -9,7 +9,7 @@
9
10
import type {LazyComponent} from 'react/src/ReactLazy';
11
12
-import {setIsRendering} from './ReactCurrentFiber';
12
+import {isRendering, setIsRendering} from './ReactCurrentFiber';
13
14
// These indirections exists so we can exclude its stack frame in DEV (and anything below it).
15
// TODO: Consider marking the whole bundle instead of these boundaries.
@@ -20,10 +20,14 @@ export function callComponentInDEV<Props, Arg, R>(
20
props: Props,
21
secondArg: Arg,
22
): R {
23
+ const wasRendering = isRendering;
24
setIsRendering(true);
24
- const result = Component(props, secondArg);
25
- setIsRendering(false);
26
- return result;
25
+ try {
26
+ const result = Component(props, secondArg);
27
+ return result;
28
+ } finally {
29
+ setIsRendering(wasRendering);
30
+ }
31
}
32
33
interface ClassInstance<R> {
@@ -32,10 +36,14 @@ interface ClassInstance<R> {
36
37
/** @noinline */
38
export function callRenderInDEV<R>(instance: ClassInstance<R>): R {
39
+ const wasRendering = isRendering;
40
setIsRendering(true);
36
- const result = instance.render();
37
- setIsRendering(false);
38
- return result;
41
+ try {
42
+ const result = instance.render();
43
+ return result;
44
+ } finally {
45
+ setIsRendering(wasRendering);
46
+ }
47
}
48
49
/** @noinline */
packages/react-reconciler/src/ReactFiberComponentStack.js
+19
-3
@@ -90,13 +90,27 @@ function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string {
90
return name ? describeBuiltInComponentFrame(name) : '';
91
}
92
93
-export function getOwnerStackByFiberInDev(workInProgress: Fiber): string {
93
+export function getOwnerStackByFiberInDev(
94
+ workInProgress: Fiber,
95
+ topStack: null | Error,
96
+): string {
97
if (!enableOwnerStacks || !__DEV__) {
98
return '';
99
}
100
try {
101
let info = '';
102
103
+ if (topStack) {
104
+ // Prefix with a filtered version of the currently executing
105
+ // stack. This information will be available in the native
106
+ // stack regardless but it's hidden since we're reprinting
107
+ // the stack on top of it.
108
+ const formattedTopStack = formatOwnerStack(topStack);
109
+ if (formattedTopStack !== '') {
110
+ info += '\n' + formattedTopStack;
111
+ }
112
+ }
113
+
114
if (workInProgress.tag === HostText) {
115
// Text nodes never have an owner/stack because they're not created through JSX.
116
// We use the parent since text nodes are always created through a host parent.
@@ -125,14 +139,16 @@ export function getOwnerStackByFiberInDev(workInProgress: Fiber): string {
139
case FunctionComponent:
140
case SimpleMemoComponent:
141
case ClassComponent:
128
- if (!workInProgress._debugOwner) {
142
+ if (!workInProgress._debugOwner && info === '') {
143
+ // Only if we have no other data about the callsite do we add
144
+ // the component name as the single stack frame.
145
info += describeFunctionComponentFrameWithoutLineNumber(
146
workInProgress.type,
147
);
148
}
149
break;
150
case ForwardRef:
135
- if (!workInProgress._debugOwner) {
151
+ if (!workInProgress._debugOwner && info === '') {
152
info += describeFunctionComponentFrameWithoutLineNumber(
153
workInProgress.type.render,
154
);
packages/react-reconciler/src/ReactFiberOwnerStack.js
+5
@@ -103,6 +103,11 @@ function filterDebugStack(error: Error): string {
103
if (lastFrameIdx !== -1) {
104
// Cut off everything after our "callComponent" slot since it'll be Fiber internals.
105
frames.length = lastFrameIdx;
106
+ } else {
107
+ // We didn't find any internal callsite out to user space.
108
+ // This means that this was called outside an owner or the owner is fully internal.
109
+ // To keep things light we exclude the entire trace in this case.
110
+ return '';
111
}
112
return frames.filter(isNotExternal).join('\n');
113
}
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
+3
-6
@@ -1618,8 +1618,7 @@ describe('ReactHooks', () => {
1618
' Previous render Next render\n' +
1619
' ------------------------------------------------------\n' +
1620
`1. ${formatHookNamesToMatchErrorMessage(hookNameA, hookNameB)}\n` +
1621
- ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' +
1622
- ' in App (at **)',
1621
+ ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
1622
]);
1623
1624
// further warnings for this component are silenced
@@ -1671,8 +1670,7 @@ describe('ReactHooks', () => {
1670
' ------------------------------------------------------\n' +
1671
`1. ${formatHookNamesToMatchErrorMessage(hookNameA, hookNameA)}\n` +
1672
`2. undefined use${hookNameB}\n` +
1674
- ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' +
1675
- ' in App (at **)',
1673
+ ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
1674
]);
1675
});
1676
});
@@ -1758,8 +1756,7 @@ describe('ReactHooks', () => {
1756
'ImperativeHandle',
1757
'Memo',
1758
)}\n` +
1761
- ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' +
1762
- ' in App (at **)',
1759
+ ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
1760
]);
1761
1762
// further warnings for this component are silenced
packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
+4
-12
@@ -228,18 +228,10 @@ describe('ReactLazy', () => {
228
229
expect(error.message).toMatch('Element type is invalid');
230
assertLog(['Loading...']);
231
- assertConsoleErrorDev(
232
- [
233
- 'Expected the result of a dynamic import() call',
234
- 'Expected the result of a dynamic import() call',
235
- ],
236
- gate(flags => flags.enableOwnerStacks)
237
- ? {
238
- // There's no owner
239
- withoutStack: true,
240
- }
241
- : undefined,
242
- );
231
+ assertConsoleErrorDev([
232
+ 'Expected the result of a dynamic import() call',
233
+ 'Expected the result of a dynamic import() call',
234
+ ]);
235
expect(root).not.toMatchRenderedOutput('Hi');
236
});
237
packages/react/src/ReactSharedInternalsClient.js
+4
-2
@@ -35,7 +35,7 @@ export type SharedStateClient = {
35
thrownErrors: Array<mixed>,
36
37
// ReactDebugCurrentFrame
38
- getCurrentStack: null | (() => string),
38
+ getCurrentStack: null | ((stack: Error) => string),
39
};
40
41
export type RendererTask = boolean => RendererTask | null;
@@ -54,7 +54,9 @@ if (__DEV__) {
54
ReactSharedInternals.didUsePromise = false;
55
ReactSharedInternals.thrownErrors = [];
56
// Stack implementation injected by the current renderer.
57
- ReactSharedInternals.getCurrentStack = (null: null | (() => string));
57
+ ReactSharedInternals.getCurrentStack = (null:
58
+ | null
59
+ | ((stack: Error) => string));
60
}
61
62
export default ReactSharedInternals;
packages/react/src/ReactSharedInternalsServer.js
+4
-2
@@ -38,7 +38,7 @@ export type SharedStateServer = {
38
// DEV-only
39
40
// ReactDebugCurrentFrame
41
- getCurrentStack: null | (() => string),
41
+ getCurrentStack: null | ((stack: Error) => string),
42
};
43
44
export type RendererTask = boolean => RendererTask | null;
@@ -58,7 +58,9 @@ if (enableTaint) {
58
59
if (__DEV__) {
60
// Stack implementation injected by the current renderer.
61
- ReactSharedInternals.getCurrentStack = (null: null | (() => string));
61
+ ReactSharedInternals.getCurrentStack = (null:
62
+ | null
63
+ | ((stack: Error) => string));
64
}
65
66
export default ReactSharedInternals;
packages/shared/consoleWithStackDev.js
+4
-4
@@ -24,7 +24,7 @@ export function setSuppressWarning(newSuppressWarning) {
24
export function warn(format, ...args) {
25
if (__DEV__) {
26
if (!suppressWarning) {
27
- printWarning('warn', format, args);
27
+ printWarning('warn', format, args, new Error('react-stack-top-frame'));
28
}
29
}
30
}
@@ -32,7 +32,7 @@ export function warn(format, ...args) {
32
export function error(format, ...args) {
33
if (__DEV__) {
34
if (!suppressWarning) {
35
- printWarning('error', format, args);
35
+ printWarning('error', format, args, new Error('react-stack-top-frame'));
36
}
37
}
38
}
@@ -40,7 +40,7 @@ export function error(format, ...args) {
40
// eslint-disable-next-line react-internal/no-production-logging
41
const supportsCreateTask = __DEV__ && enableOwnerStacks && !!console.createTask;
42
43
-function printWarning(level, format, args) {
43
+function printWarning(level, format, args, currentStack) {
44
// When changing this logic, you might want to also
45
// update consoleWithStackDev.www.js as well.
46
if (__DEV__) {
@@ -51,7 +51,7 @@ function printWarning(level, format, args) {
51
// We only add the current stack to the console when createTask is not supported.
52
// Since createTask requires DevTools to be open to work, this means that stacks
53
// can be lost while DevTools isn't open but we can't detect this.
54
- const stack = ReactSharedInternals.getCurrentStack();
54
+ const stack = ReactSharedInternals.getCurrentStack(currentStack);
55
if (stack !== '') {
56
format += '%s';
57
args = args.concat([stack]);
packages/shared/forks/consoleWithStackDev.www.js
+4
-4
@@ -18,7 +18,7 @@ export function setSuppressWarning(newSuppressWarning) {
18
export function warn(format, ...args) {
19
if (__DEV__) {
20
if (!suppressWarning) {
21
- printWarning('warn', format, args);
21
+ printWarning('warn', format, args, new Error('react-stack-top-frame'));
22
}
23
}
24
}
@@ -26,19 +26,19 @@ export function warn(format, ...args) {
26
export function error(format, ...args) {
27
if (__DEV__) {
28
if (!suppressWarning) {
29
- printWarning('error', format, args);
29
+ printWarning('error', format, args, new Error('react-stack-top-frame'));
30
}
31
}
32
}
33
34
-function printWarning(level, format, args) {
34
+function printWarning(level, format, args, currentStack) {
35
if (__DEV__) {
36
const React = require('react');
37
const ReactSharedInternals =
38
React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE;
39
// Defensive in case this is fired before React is initialized.
40
if (ReactSharedInternals != null && ReactSharedInternals.getCurrentStack) {
41
- const stack = ReactSharedInternals.getCurrentStack();
41
+ const stack = ReactSharedInternals.getCurrentStack(currentStack);
42
if (stack !== '') {
43
format += '%s';
44
args.push(stack);