Skip to content

Commit

Permalink
Stop filtering owner stacks (#30438)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sebmarkbage authored Jul 24, 2024
1 parent ab2135c commit 5b37af7
Show file tree
Hide file tree
Showing 11 changed files with 128 additions and 212 deletions.
29 changes: 9 additions & 20 deletions packages/react-devtools-shared/src/backend/DevToolsOwnerStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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' +
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberComponentStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
47 changes: 0 additions & 47 deletions packages/react-reconciler/src/ReactFiberOwnerStack.js

This file was deleted.

2 changes: 1 addition & 1 deletion packages/react-server/src/ReactFizzComponentStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
47 changes: 0 additions & 47 deletions packages/react-server/src/ReactFizzOwnerStack.js

This file was deleted.

36 changes: 1 addition & 35 deletions packages/react-server/src/ReactFlightCallUserSpace.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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);
}
Expand All @@ -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)
Expand Down
42 changes: 0 additions & 42 deletions packages/react-server/src/ReactFlightOwnerStack.js

This file was deleted.

92 changes: 74 additions & 18 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ import {
createHints,
initAsyncDebugInfo,
parseStackTrace,
supportsComponentStorage,
componentStorage,
} from './ReactFlightServerConfig';

import {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 5b37af7

Please sign in to comment.