From 433068eece2071a96de98b60f99ce6a9121a629c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 11 Jul 2024 18:34:41 -0400 Subject: [PATCH] Remove top stack frame from getCurrentStack (#30306) The full stack is the current execution stack (`new Error().stack`) + the current owner stack (`React.captureOwnerStack()`). The idea with the top frame was that when we append it to console.error we'd include both since otherwise the true reason would be obscured behind the little `>` to expand. So we'd just put both stack front and center. By adding this into getCurrentStack it was easy to use the same filtering. I never implemented in Fizz or Flight though. However, with the public API `React.captureOwnerStack()` it's not necessary to include the current stack since you already have it and you'd have filtering capabilities in user space too. Since I'm removing the component stacks from React itself we no longer need this. It's expected that maybe RDT or framework polyfill would include this same technique though. --- .../react-reconciler/src/ReactCurrentFiber.js | 4 ++-- .../src/ReactFiberComponentStack.js | 16 +--------------- .../src/__tests__/ReactLazy-test.internal.js | 14 +++++++++----- packages/react/src/ReactOwnerStack.js | 2 +- packages/react/src/ReactSharedInternalsClient.js | 6 ++---- packages/react/src/ReactSharedInternalsServer.js | 6 ++---- packages/shared/forks/consoleWithStackDev.rn.js | 8 ++++---- packages/shared/forks/consoleWithStackDev.www.js | 8 ++++---- 8 files changed, 25 insertions(+), 39 deletions(-) diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index 29431a8a0a..18406d9919 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -33,7 +33,7 @@ export function getCurrentFiberOwnerNameInDevOrNull(): string | null { return null; } -function getCurrentFiberStackInDev(stack: null | Error): string { +function getCurrentFiberStackInDev(): string { if (__DEV__) { if (current === null) { return ''; @@ -43,7 +43,7 @@ function getCurrentFiberStackInDev(stack: null | Error): 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, stack); + return getOwnerStackByFiberInDev(current); } return getStackByFiberInDevAndProd(current); } diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index a06411acad..a3063e2b21 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -91,27 +91,13 @@ function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string { return name ? describeBuiltInComponentFrame(name) : ''; } -export function getOwnerStackByFiberInDev( - workInProgress: Fiber, - topStack: null | Error, -): string { +export function getOwnerStackByFiberInDev(workInProgress: Fiber): 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. diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index d4eeb4f13f..644913a15f 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -213,14 +213,18 @@ describe('ReactLazy', () => { unstable_isConcurrent: true, }); + function App() { + return ( + }> + + + ); + } + let error; try { await act(() => { - root.update( - }> - - , - ); + root.update(); }); } catch (e) { error = e; diff --git a/packages/react/src/ReactOwnerStack.js b/packages/react/src/ReactOwnerStack.js index f6944b08cd..4746c3a7c6 100644 --- a/packages/react/src/ReactOwnerStack.js +++ b/packages/react/src/ReactOwnerStack.js @@ -20,5 +20,5 @@ export function captureOwnerStack(): null | string { } // The current stack will be the owner stack if enableOwnerStacks is true // which it is always here. Otherwise it's the parent stack. - return getCurrentStack(null); + return getCurrentStack(); } diff --git a/packages/react/src/ReactSharedInternalsClient.js b/packages/react/src/ReactSharedInternalsClient.js index 8b00f98a51..452bd933da 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 | ((stack: null | Error) => string), + getCurrentStack: null | (() => string), }; export type RendererTask = boolean => RendererTask | null; @@ -54,9 +54,7 @@ if (__DEV__) { ReactSharedInternals.didUsePromise = false; ReactSharedInternals.thrownErrors = []; // Stack implementation injected by the current renderer. - ReactSharedInternals.getCurrentStack = (null: - | null - | ((stack: null | Error) => string)); + ReactSharedInternals.getCurrentStack = (null: null | (() => string)); } export default ReactSharedInternals; diff --git a/packages/react/src/ReactSharedInternalsServer.js b/packages/react/src/ReactSharedInternalsServer.js index 0bfeb26fb8..d670fa18fe 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 | ((stack: null | Error) => string), + getCurrentStack: null | (() => string), }; export type RendererTask = boolean => RendererTask | null; @@ -58,9 +58,7 @@ if (enableTaint) { if (__DEV__) { // Stack implementation injected by the current renderer. - ReactSharedInternals.getCurrentStack = (null: - | null - | ((stack: null | Error) => string)); + ReactSharedInternals.getCurrentStack = (null: null | (() => string)); } export default ReactSharedInternals; diff --git a/packages/shared/forks/consoleWithStackDev.rn.js b/packages/shared/forks/consoleWithStackDev.rn.js index 6b567a446a..44767992cd 100644 --- a/packages/shared/forks/consoleWithStackDev.rn.js +++ b/packages/shared/forks/consoleWithStackDev.rn.js @@ -23,7 +23,7 @@ export function setSuppressWarning(newSuppressWarning) { export function warn(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('warn', format, args, new Error('react-stack-top-frame')); + printWarning('warn', format, args); } } } @@ -31,17 +31,17 @@ export function warn(format, ...args) { export function error(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('error', format, args, new Error('react-stack-top-frame')); + printWarning('error', format, args); } } } export let isWritingAppendedStack = false; -function printWarning(level, format, args, currentStack) { +function printWarning(level, format, args) { if (__DEV__) { if (ReactSharedInternals.getCurrentStack) { - const stack = ReactSharedInternals.getCurrentStack(currentStack); + const stack = ReactSharedInternals.getCurrentStack(); if (stack !== '') { isWritingAppendedStack = true; format += '%s'; diff --git a/packages/shared/forks/consoleWithStackDev.www.js b/packages/shared/forks/consoleWithStackDev.www.js index 5f04f36359..c4311efe09 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, new Error('react-stack-top-frame')); + printWarning('warn', format, args); } } } @@ -26,19 +26,19 @@ export function warn(format, ...args) { export function error(format, ...args) { if (__DEV__) { if (!suppressWarning) { - printWarning('error', format, args, new Error('react-stack-top-frame')); + printWarning('error', format, args); } } } -function printWarning(level, format, args, currentStack) { +function printWarning(level, format, args) { 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(currentStack); + const stack = ReactSharedInternals.getCurrentStack(); if (stack !== '') { format += '%s'; args.push(stack);