From 5b37af7daa896ef21eec2db2fe9bc8ad42a1a328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 24 Jul 2024 13:01:07 -0400 Subject: [PATCH] Stop filtering owner stacks (#30438) We still filter them before passing from server to client in Flight Server but when presenting a native stack, we don't need to filter them. That's left to ignore listing in the presentation. The stacks are pretty clean regardless thanks to the bottom stack frames. We can also unify the owner stack formatters into one shared module since Fizz/Flight/Fiber all do the same thing. DevTools currently does the same thing but is forked so it can support multiple versions. --- .../src/backend/DevToolsOwnerStack.js | 29 ++---- .../src/__tests__/ReactDOMFizzServer-test.js | 1 + .../src/ReactFiberComponentStack.js | 2 +- .../src/ReactFiberOwnerStack.js | 47 ---------- .../src/ReactFizzComponentStack.js | 2 +- .../react-server/src/ReactFizzOwnerStack.js | 47 ---------- .../src/ReactFlightCallUserSpace.js | 36 +------- .../react-server/src/ReactFlightOwnerStack.js | 42 --------- .../react-server/src/ReactFlightServer.js | 92 +++++++++++++++---- .../src/flight/ReactFlightComponentStack.js | 2 +- packages/shared/ReactOwnerStackFrames.js | 40 ++++++++ 11 files changed, 128 insertions(+), 212 deletions(-) delete mode 100644 packages/react-reconciler/src/ReactFiberOwnerStack.js delete mode 100644 packages/react-server/src/ReactFizzOwnerStack.js delete mode 100644 packages/react-server/src/ReactFlightOwnerStack.js create mode 100644 packages/shared/ReactOwnerStackFrames.js diff --git a/packages/react-devtools-shared/src/backend/DevToolsOwnerStack.js b/packages/react-devtools-shared/src/backend/DevToolsOwnerStack.js index 5a859ed95f..e03d948a45 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsOwnerStack.js +++ b/packages/react-devtools-shared/src/backend/DevToolsOwnerStack.js @@ -7,20 +7,9 @@ * @flow */ -// This is a DevTools fork of ReactFiberOwnerStack. +// This is a DevTools fork of shared/ReactOwnerStackFrames. -// TODO: Make this configurable? -const externalRegExp = /\/node\_modules\/|\(\/; - -function isNotExternal(stackFrame: string): boolean { - return !externalRegExp.test(stackFrame); -} - -function filterDebugStack(error: Error): string { - // Since stacks can be quite large and we pass a lot of them, we filter them out eagerly - // to save bandwidth even in DEV. We'll also replay these stacks on the client so by - // stripping them early we avoid that overhead. Otherwise we'd normally just rely on - // the DevTools or framework's ignore lists to filter them out. +export function formatOwnerStack(error: Error): string { const prevPrepareStackTrace = Error.prepareStackTrace; // $FlowFixMe[incompatible-type] It does accept undefined. Error.prepareStackTrace = undefined; @@ -31,7 +20,12 @@ function filterDebugStack(error: Error): string { // don't want/need. stack = stack.slice(29); } - let idx = stack.indexOf('react-stack-bottom-frame'); + let idx = stack.indexOf('\n'); + if (idx !== -1) { + // Pop the JSX frame. + stack = stack.slice(idx + 1); + } + idx = stack.indexOf('react-stack-bottom-frame'); if (idx !== -1) { idx = stack.lastIndexOf('\n', idx); } @@ -44,10 +38,5 @@ function filterDebugStack(error: Error): string { // To keep things light we exclude the entire trace in this case. return ''; } - const frames = stack.split('\n').slice(1); // Pop the JSX frame. - return frames.filter(isNotExternal).join('\n'); -} - -export function formatOwnerStack(ownerStackTrace: Error): string { - return filterDebugStack(ownerStackTrace); + return stack; } diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index f51101b4d4..ced32ff87e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1848,6 +1848,7 @@ describe('ReactDOMFizzServer', () => { (gate(flags => flags.enableOwnerStacks) ? ' in span (at **)\n' + ' in mapper (at **)\n' + + ' in Array.map (at **)\n' + ' in B (at **)\n' + ' in A (at **)' : ' in span (at **)\n' + diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index a24db0c8d1..ac2cdf20a4 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -30,7 +30,7 @@ import { describeClassComponentFrame, describeDebugInfoFrame, } from 'shared/ReactComponentStackFrame'; -import {formatOwnerStack} from './ReactFiberOwnerStack'; +import {formatOwnerStack} from 'shared/ReactOwnerStackFrames'; function describeFiber(fiber: Fiber): string { switch (fiber.tag) { diff --git a/packages/react-reconciler/src/ReactFiberOwnerStack.js b/packages/react-reconciler/src/ReactFiberOwnerStack.js deleted file mode 100644 index fbca1c2de3..0000000000 --- a/packages/react-reconciler/src/ReactFiberOwnerStack.js +++ /dev/null @@ -1,47 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -// TODO: Make this configurable on the root. -const externalRegExp = /\/node\_modules\/|\(\/; - -function isNotExternal(stackFrame: string): boolean { - return !externalRegExp.test(stackFrame); -} - -function filterDebugStack(error: Error): string { - // Since stacks can be quite large and we pass a lot of them, we filter them out eagerly - // to save bandwidth even in DEV. We'll also replay these stacks on the client so by - // stripping them early we avoid that overhead. Otherwise we'd normally just rely on - // the DevTools or framework's ignore lists to filter them out. - let stack = error.stack; - if (stack.startsWith('Error: react-stack-top-frame\n')) { - // V8's default formatting prefixes with the error message which we - // don't want/need. - stack = stack.slice(29); - } - let idx = stack.indexOf('react-stack-bottom-frame'); - if (idx !== -1) { - idx = stack.lastIndexOf('\n', idx); - } - if (idx !== -1) { - // Cut off everything after the bottom frame since it'll be internals. - stack = stack.slice(0, idx); - } 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 ''; - } - const frames = stack.split('\n').slice(1); // Pop the JSX frame. - return frames.filter(isNotExternal).join('\n'); -} - -export function formatOwnerStack(ownerStackTrace: Error): string { - return filterDebugStack(ownerStackTrace); -} diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js index ab306047bf..7bec67038c 100644 --- a/packages/react-server/src/ReactFizzComponentStack.js +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -27,7 +27,7 @@ import { import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; -import {formatOwnerStack} from './ReactFizzOwnerStack'; +import {formatOwnerStack} from 'shared/ReactOwnerStackFrames'; export type ComponentStackNode = { parent: null | ComponentStackNode, diff --git a/packages/react-server/src/ReactFizzOwnerStack.js b/packages/react-server/src/ReactFizzOwnerStack.js deleted file mode 100644 index fbca1c2de3..0000000000 --- a/packages/react-server/src/ReactFizzOwnerStack.js +++ /dev/null @@ -1,47 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -// TODO: Make this configurable on the root. -const externalRegExp = /\/node\_modules\/|\(\/; - -function isNotExternal(stackFrame: string): boolean { - return !externalRegExp.test(stackFrame); -} - -function filterDebugStack(error: Error): string { - // Since stacks can be quite large and we pass a lot of them, we filter them out eagerly - // to save bandwidth even in DEV. We'll also replay these stacks on the client so by - // stripping them early we avoid that overhead. Otherwise we'd normally just rely on - // the DevTools or framework's ignore lists to filter them out. - let stack = error.stack; - if (stack.startsWith('Error: react-stack-top-frame\n')) { - // V8's default formatting prefixes with the error message which we - // don't want/need. - stack = stack.slice(29); - } - let idx = stack.indexOf('react-stack-bottom-frame'); - if (idx !== -1) { - idx = stack.lastIndexOf('\n', idx); - } - if (idx !== -1) { - // Cut off everything after the bottom frame since it'll be internals. - stack = stack.slice(0, idx); - } 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 ''; - } - const frames = stack.split('\n').slice(1); // Pop the JSX frame. - return frames.filter(isNotExternal).join('\n'); -} - -export function formatOwnerStack(ownerStackTrace: Error): string { - return filterDebugStack(ownerStackTrace); -} diff --git a/packages/react-server/src/ReactFlightCallUserSpace.js b/packages/react-server/src/ReactFlightCallUserSpace.js index 01c0492cc3..4f7e065153 100644 --- a/packages/react-server/src/ReactFlightCallUserSpace.js +++ b/packages/react-server/src/ReactFlightCallUserSpace.js @@ -15,13 +15,6 @@ import type {ReactClientValue} from './ReactFlightServer'; import {setCurrentOwner} from './flight/ReactFlightCurrentOwner'; -import { - supportsComponentStorage, - componentStorage, -} from './ReactFlightServerConfig'; - -import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; - // 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. @@ -30,38 +23,12 @@ const callComponent = { Component: (p: Props, arg: void) => R, props: Props, componentDebugInfo: ReactComponentInfo, - debugTask: null | ConsoleTask, ): R { // The secondArg is always undefined in Server Components since refs error early. const secondArg = undefined; setCurrentOwner(componentDebugInfo); try { - if (supportsComponentStorage) { - // Run the component in an Async Context that tracks the current owner. - if (enableOwnerStacks && debugTask) { - return debugTask.run( - // $FlowFixMe[method-unbinding] - componentStorage.run.bind( - componentStorage, - componentDebugInfo, - Component, - props, - secondArg, - ), - ); - } - return componentStorage.run( - componentDebugInfo, - Component, - props, - secondArg, - ); - } else { - if (enableOwnerStacks && debugTask) { - return debugTask.run(Component.bind(null, props, secondArg)); - } - return Component(props, secondArg); - } + return Component(props, secondArg); } finally { setCurrentOwner(null); } @@ -72,7 +39,6 @@ export const callComponentInDEV: ( Component: (p: Props, arg: void) => R, props: Props, componentDebugInfo: ReactComponentInfo, - debugTask: null | ConsoleTask, ) => R = __DEV__ ? // We use this technique to trick minifiers to preserve the function name. (callComponent['react-stack-bottom-frame'].bind(callComponent): any) diff --git a/packages/react-server/src/ReactFlightOwnerStack.js b/packages/react-server/src/ReactFlightOwnerStack.js deleted file mode 100644 index 72951bfb47..0000000000 --- a/packages/react-server/src/ReactFlightOwnerStack.js +++ /dev/null @@ -1,42 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -// TODO: Make this configurable on the Request. -const externalRegExp = /\/node\_modules\/| \(node\:| node\:|\(\/; - -function isNotExternal(stackFrame: string): boolean { - return !externalRegExp.test(stackFrame); -} - -function filterDebugStack(error: Error): string { - // Since stacks can be quite large and we pass a lot of them, we filter them out eagerly - // to save bandwidth even in DEV. We'll also replay these stacks on the client so by - // stripping them early we avoid that overhead. Otherwise we'd normally just rely on - // the DevTools or framework's ignore lists to filter them out. - let stack = error.stack; - if (stack.startsWith('Error: react-stack-top-frame\n')) { - // V8's default formatting prefixes with the error message which we - // don't want/need. - stack = stack.slice(29); - } - let idx = stack.indexOf('react-stack-bottom-frame'); - if (idx !== -1) { - idx = stack.lastIndexOf('\n', idx); - } - if (idx !== -1) { - // Cut off everything after the bottom frame since it'll be internals. - stack = stack.slice(0, idx); - } - const frames = stack.split('\n').slice(1); // Pop the JSX frame. - return frames.filter(isNotExternal).join('\n'); -} - -export function formatOwnerStack(ownerStackTrace: Error): string { - return filterDebugStack(ownerStackTrace); -} diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index fe2b258dd0..ae031c6521 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -80,6 +80,8 @@ import { createHints, initAsyncDebugInfo, parseStackTrace, + supportsComponentStorage, + componentStorage, } from './ReactFlightServerConfig'; import { @@ -1035,12 +1037,38 @@ function renderFunctionComponent( } } prepareToUseHooksForComponent(prevThenableState, componentDebugInfo); - result = callComponentInDEV( - Component, - props, - componentDebugInfo, - task.debugTask, - ); + if (supportsComponentStorage) { + // Run the component in an Async Context that tracks the current owner. + if (enableOwnerStacks && task.debugTask) { + result = task.debugTask.run( + // $FlowFixMe[method-unbinding] + componentStorage.run.bind( + componentStorage, + componentDebugInfo, + callComponentInDEV, + Component, + props, + componentDebugInfo, + ), + ); + } else { + result = componentStorage.run( + componentDebugInfo, + callComponentInDEV, + Component, + props, + componentDebugInfo, + ); + } + } else { + if (enableOwnerStacks && task.debugTask) { + result = task.debugTask.run( + callComponentInDEV.bind(null, Component, props, componentDebugInfo), + ); + } else { + result = callComponentInDEV(Component, props, componentDebugInfo); + } + } } else { prepareToUseHooksForComponent(prevThenableState, null); // The secondArg is always undefined in Server Components since refs error early. @@ -1222,19 +1250,47 @@ function warnForMissingKey( // Call with the server component as the currently rendering component // for context. - callComponentInDEV( - () => { - console.error( - 'Each child in a list should have a unique "key" prop.' + - '%s%s See https://react.dev/link/warning-keys for more information.', - '', - '', + const logKeyError = () => { + console.error( + 'Each child in a list should have a unique "key" prop.' + + '%s%s See https://react.dev/link/warning-keys for more information.', + '', + '', + ); + }; + + if (supportsComponentStorage) { + // Run the component in an Async Context that tracks the current owner. + if (enableOwnerStacks && debugTask) { + debugTask.run( + // $FlowFixMe[method-unbinding] + componentStorage.run.bind( + componentStorage, + componentDebugInfo, + callComponentInDEV, + logKeyError, + null, + componentDebugInfo, + ), ); - }, - null, - componentDebugInfo, - debugTask, - ); + } else { + componentStorage.run( + componentDebugInfo, + callComponentInDEV, + logKeyError, + null, + componentDebugInfo, + ); + } + } else { + if (enableOwnerStacks && debugTask) { + debugTask.run( + callComponentInDEV.bind(null, logKeyError, null, componentDebugInfo), + ); + } else { + callComponentInDEV(logKeyError, null, componentDebugInfo); + } + } } } diff --git a/packages/react-server/src/flight/ReactFlightComponentStack.js b/packages/react-server/src/flight/ReactFlightComponentStack.js index 36ee51dcf0..e4e545edae 100644 --- a/packages/react-server/src/flight/ReactFlightComponentStack.js +++ b/packages/react-server/src/flight/ReactFlightComponentStack.js @@ -13,7 +13,7 @@ import {describeBuiltInComponentFrame} from 'shared/ReactComponentStackFrame'; import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; -import {formatOwnerStack} from '../ReactFlightOwnerStack'; +import {formatOwnerStack} from 'shared/ReactOwnerStackFrames'; export function getOwnerStackByComponentInfoInDev( componentInfo: ReactComponentInfo, diff --git a/packages/shared/ReactOwnerStackFrames.js b/packages/shared/ReactOwnerStackFrames.js new file mode 100644 index 0000000000..829930430a --- /dev/null +++ b/packages/shared/ReactOwnerStackFrames.js @@ -0,0 +1,40 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export function formatOwnerStack(error: Error): string { + const prevPrepareStackTrace = Error.prepareStackTrace; + // $FlowFixMe[incompatible-type] It does accept undefined. + Error.prepareStackTrace = undefined; + let stack = error.stack; + Error.prepareStackTrace = prevPrepareStackTrace; + if (stack.startsWith('Error: react-stack-top-frame\n')) { + // V8's default formatting prefixes with the error message which we + // don't want/need. + stack = stack.slice(29); + } + let idx = stack.indexOf('\n'); + if (idx !== -1) { + // Pop the JSX frame. + stack = stack.slice(idx + 1); + } + idx = stack.indexOf('react-stack-bottom-frame'); + if (idx !== -1) { + idx = stack.lastIndexOf('\n', idx); + } + if (idx !== -1) { + // Cut off everything after the bottom frame since it'll be internals. + stack = stack.slice(0, idx); + } 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 stack; +}