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.
This commit is contained in:
Sebastian Markbåge 2024-07-24 13:01:07 -04:00 committed by GitHub
parent ab2135c708
commit 5b37af7daa
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 128 additions and 212 deletions

View File

@ -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\/|\(\<anonymous\>/;
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;
}

View File

@ -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' +

View File

@ -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) {

View File

@ -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\/|\(\<anonymous\>/;
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);
}

View File

@ -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,

View File

@ -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\/|\(\<anonymous\>/;
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);
}

View File

@ -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: <Props, R>(
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)

View File

@ -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\:|\(\<anonymous\>/;
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);
}

View File

@ -80,6 +80,8 @@ import {
createHints,
initAsyncDebugInfo,
parseStackTrace,
supportsComponentStorage,
componentStorage,
} from './ReactFlightServerConfig';
import {
@ -1035,12 +1037,38 @@ function renderFunctionComponent<Props>(
}
}
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);
}
}
}
}

View File

@ -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,

View File

@ -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;
}