From 89b976f35d592428e21e87c1d37da10db745ecbe Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 31 May 2024 18:13:27 -0400 Subject: [PATCH] Prefix owner stacks with the current stack at the console call 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. --- .../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 cf0c2543a57..fd2b4e7d802 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 dfc88b64be8..e85d0431b99 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 e5e25f67469..8a69ba9ddfd 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 b8510ebf6bc..fe9e4f1cfd6 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 8cc5c15a9e0..fd1879dabfb 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 3ec58b2f705..73f9aa9cc5d 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 452bd933dab..6a54c73be30 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 d670fa18fe7..749ce5c3ad4 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 bdcf7548021..4638ede81cb 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 c4311efe09f..5f04f36359f 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);