From 4dcdf21325028d7ae9bb3c2172dbbe9647a744ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 3 Jun 2024 12:26:38 -0400 Subject: [PATCH] [Fiber] Prefix owner stacks with the current stack at the console call (#29697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Screenshot 2024-05-31 at 6 15 27 PM 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.) --- .../react-reconciler/src/ReactCurrentFiber.js | 4 ++-- .../src/ReactFiberCallUserSpace.js | 22 +++++++++++++------ .../src/ReactFiberComponentStack.js | 22 ++++++++++++++++--- .../src/ReactFiberOwnerStack.js | 5 +++++ .../src/__tests__/ReactHooks-test.internal.js | 9 +++----- .../src/__tests__/ReactLazy-test.internal.js | 16 ++++---------- .../react/src/ReactSharedInternalsClient.js | 6 +++-- .../react/src/ReactSharedInternalsServer.js | 6 +++-- packages/shared/consoleWithStackDev.js | 8 +++---- .../shared/forks/consoleWithStackDev.www.js | 8 +++---- 10 files changed, 64 insertions(+), 42 deletions(-) diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index cf0c2543a5..fd2b4e7d80 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -44,7 +44,7 @@ export function getCurrentParentStackInDev(): string { return ''; } -function getCurrentFiberStackInDev(): string { +function getCurrentFiberStackInDev(stack: Error): string { if (__DEV__) { if (current === null) { return ''; @@ -54,7 +54,7 @@ function getCurrentFiberStackInDev(): string { // TODO: The above comment is not actually true. We might be // in a commit phase or preemptive set state callback. if (enableOwnerStacks) { - return getOwnerStackByFiberInDev(current); + return getOwnerStackByFiberInDev(current, stack); } return getStackByFiberInDevAndProd(current); } diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js index dfc88b64be..e85d0431b9 100644 --- a/packages/react-reconciler/src/ReactFiberCallUserSpace.js +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -9,7 +9,7 @@ import type {LazyComponent} from 'react/src/ReactLazy'; -import {setIsRendering} from './ReactCurrentFiber'; +import {isRendering, setIsRendering} from './ReactCurrentFiber'; // These indirections exists so we can exclude its stack frame in DEV (and anything below it). // TODO: Consider marking the whole bundle instead of these boundaries. @@ -20,10 +20,14 @@ export function callComponentInDEV( props: Props, secondArg: Arg, ): R { + const wasRendering = isRendering; setIsRendering(true); - const result = Component(props, secondArg); - setIsRendering(false); - return result; + try { + const result = Component(props, secondArg); + return result; + } finally { + setIsRendering(wasRendering); + } } interface ClassInstance { @@ -32,10 +36,14 @@ interface ClassInstance { /** @noinline */ export function callRenderInDEV(instance: ClassInstance): R { + const wasRendering = isRendering; setIsRendering(true); - const result = instance.render(); - setIsRendering(false); - return result; + try { + const result = instance.render(); + return result; + } finally { + setIsRendering(wasRendering); + } } /** @noinline */ diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index e5e25f6746..8a69ba9ddf 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -90,13 +90,27 @@ function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string { return name ? describeBuiltInComponentFrame(name) : ''; } -export function getOwnerStackByFiberInDev(workInProgress: Fiber): string { +export function getOwnerStackByFiberInDev( + workInProgress: Fiber, + topStack: null | Error, +): string { if (!enableOwnerStacks || !__DEV__) { return ''; } try { let info = ''; + if (topStack) { + // Prefix with a filtered version of the currently executing + // stack. This information will be available in the native + // stack regardless but it's hidden since we're reprinting + // the stack on top of it. + const formattedTopStack = formatOwnerStack(topStack); + if (formattedTopStack !== '') { + info += '\n' + formattedTopStack; + } + } + if (workInProgress.tag === HostText) { // Text nodes never have an owner/stack because they're not created through JSX. // We use the parent since text nodes are always created through a host parent. @@ -125,14 +139,16 @@ export function getOwnerStackByFiberInDev(workInProgress: Fiber): string { case FunctionComponent: case SimpleMemoComponent: case ClassComponent: - if (!workInProgress._debugOwner) { + 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) { + if (!workInProgress._debugOwner && info === '') { info += describeFunctionComponentFrameWithoutLineNumber( workInProgress.type.render, ); diff --git a/packages/react-reconciler/src/ReactFiberOwnerStack.js b/packages/react-reconciler/src/ReactFiberOwnerStack.js index b8510ebf6b..fe9e4f1cfd 100644 --- a/packages/react-reconciler/src/ReactFiberOwnerStack.js +++ b/packages/react-reconciler/src/ReactFiberOwnerStack.js @@ -103,6 +103,11 @@ function filterDebugStack(error: Error): string { if (lastFrameIdx !== -1) { // Cut off everything after our "callComponent" slot since it'll be Fiber internals. frames.length = lastFrameIdx; + } else { + // We didn't find any internal callsite out to user space. + // This means that this was called outside an owner or the owner is fully internal. + // To keep things light we exclude the entire trace in this case. + return ''; } return frames.filter(isNotExternal).join('\n'); } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 8cc5c15a9e..fd1879dabf 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1618,8 +1618,7 @@ describe('ReactHooks', () => { ' Previous render Next render\n' + ' ------------------------------------------------------\n' + `1. ${formatHookNamesToMatchErrorMessage(hookNameA, hookNameB)}\n` + - ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' + - ' in App (at **)', + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n', ]); // further warnings for this component are silenced @@ -1671,8 +1670,7 @@ describe('ReactHooks', () => { ' ------------------------------------------------------\n' + `1. ${formatHookNamesToMatchErrorMessage(hookNameA, hookNameA)}\n` + `2. undefined use${hookNameB}\n` + - ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' + - ' in App (at **)', + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n', ]); }); }); @@ -1758,8 +1756,7 @@ describe('ReactHooks', () => { 'ImperativeHandle', 'Memo', )}\n` + - ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' + - ' in App (at **)', + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n', ]); // further warnings for this component are silenced diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 3ec58b2f70..73f9aa9cc5 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -228,18 +228,10 @@ describe('ReactLazy', () => { expect(error.message).toMatch('Element type is invalid'); assertLog(['Loading...']); - assertConsoleErrorDev( - [ - 'Expected the result of a dynamic import() call', - 'Expected the result of a dynamic import() call', - ], - gate(flags => flags.enableOwnerStacks) - ? { - // There's no owner - withoutStack: true, - } - : undefined, - ); + assertConsoleErrorDev([ + 'Expected the result of a dynamic import() call', + 'Expected the result of a dynamic import() call', + ]); expect(root).not.toMatchRenderedOutput('Hi'); }); diff --git a/packages/react/src/ReactSharedInternalsClient.js b/packages/react/src/ReactSharedInternalsClient.js index 452bd933da..6a54c73be3 100644 --- a/packages/react/src/ReactSharedInternalsClient.js +++ b/packages/react/src/ReactSharedInternalsClient.js @@ -35,7 +35,7 @@ export type SharedStateClient = { thrownErrors: Array, // ReactDebugCurrentFrame - getCurrentStack: null | (() => string), + getCurrentStack: null | ((stack: Error) => string), }; export type RendererTask = boolean => RendererTask | null; @@ -54,7 +54,9 @@ if (__DEV__) { ReactSharedInternals.didUsePromise = false; ReactSharedInternals.thrownErrors = []; // Stack implementation injected by the current renderer. - ReactSharedInternals.getCurrentStack = (null: null | (() => string)); + ReactSharedInternals.getCurrentStack = (null: + | null + | ((stack: Error) => string)); } export default ReactSharedInternals; diff --git a/packages/react/src/ReactSharedInternalsServer.js b/packages/react/src/ReactSharedInternalsServer.js index d670fa18fe..749ce5c3ad 100644 --- a/packages/react/src/ReactSharedInternalsServer.js +++ b/packages/react/src/ReactSharedInternalsServer.js @@ -38,7 +38,7 @@ export type SharedStateServer = { // DEV-only // ReactDebugCurrentFrame - getCurrentStack: null | (() => string), + getCurrentStack: null | ((stack: Error) => string), }; export type RendererTask = boolean => RendererTask | null; @@ -58,7 +58,9 @@ if (enableTaint) { if (__DEV__) { // Stack implementation injected by the current renderer. - ReactSharedInternals.getCurrentStack = (null: null | (() => string)); + ReactSharedInternals.getCurrentStack = (null: + | null + | ((stack: Error) => string)); } export default ReactSharedInternals; diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js index bdcf754802..4638ede81c 100644 --- a/packages/shared/consoleWithStackDev.js +++ b/packages/shared/consoleWithStackDev.js @@ -24,7 +24,7 @@ export function setSuppressWarning(newSuppressWarning) { export function warn(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('warn', format, args); + printWarning('warn', format, args, new Error('react-stack-top-frame')); } } } @@ -32,7 +32,7 @@ export function warn(format, ...args) { export function error(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('error', format, args); + printWarning('error', format, args, new Error('react-stack-top-frame')); } } } @@ -40,7 +40,7 @@ export function error(format, ...args) { // eslint-disable-next-line react-internal/no-production-logging const supportsCreateTask = __DEV__ && enableOwnerStacks && !!console.createTask; -function printWarning(level, format, args) { +function printWarning(level, format, args, currentStack) { // When changing this logic, you might want to also // update consoleWithStackDev.www.js as well. if (__DEV__) { @@ -51,7 +51,7 @@ function printWarning(level, format, args) { // We only add the current stack to the console when createTask is not supported. // Since createTask requires DevTools to be open to work, this means that stacks // can be lost while DevTools isn't open but we can't detect this. - const stack = ReactSharedInternals.getCurrentStack(); + const stack = ReactSharedInternals.getCurrentStack(currentStack); if (stack !== '') { format += '%s'; args = args.concat([stack]); diff --git a/packages/shared/forks/consoleWithStackDev.www.js b/packages/shared/forks/consoleWithStackDev.www.js index c4311efe09..5f04f36359 100644 --- a/packages/shared/forks/consoleWithStackDev.www.js +++ b/packages/shared/forks/consoleWithStackDev.www.js @@ -18,7 +18,7 @@ export function setSuppressWarning(newSuppressWarning) { export function warn(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('warn', format, args); + printWarning('warn', format, args, new Error('react-stack-top-frame')); } } } @@ -26,19 +26,19 @@ export function warn(format, ...args) { export function error(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('error', format, args); + printWarning('error', format, args, new Error('react-stack-top-frame')); } } } -function printWarning(level, format, args) { +function printWarning(level, format, args, currentStack) { if (__DEV__) { const React = require('react'); const ReactSharedInternals = React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE; // Defensive in case this is fired before React is initialized. if (ReactSharedInternals != null && ReactSharedInternals.getCurrentStack) { - const stack = ReactSharedInternals.getCurrentStack(); + const stack = ReactSharedInternals.getCurrentStack(currentStack); if (stack !== '') { format += '%s'; args.push(stack);