Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove top stack frame from getCurrentStack #30306

Merged
merged 1 commit into from
Jul 11, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactCurrentFiber.js
Original file line number Diff line number Diff line change
@@ -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);
}
16 changes: 1 addition & 15 deletions packages/react-reconciler/src/ReactFiberComponentStack.js
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -213,14 +213,18 @@ describe('ReactLazy', () => {
unstable_isConcurrent: true,
});

function App() {
return (
<Suspense fallback={<Text text="Loading..." />}>
<LazyText text="Hi" />
</Suspense>
);
}

let error;
try {
await act(() => {
root.update(
<Suspense fallback={<Text text="Loading..." />}>
<LazyText text="Hi" />
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a lot of test like this that execute at the top level and so has no owner. Therefore doesn't get any owner stack which means that it doesn't have a component stack which fails our assertions. This is why I usually try to add a single frame if possible. Mainly due to our tests.

https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberComponentStack.js#L143-L156

This test was relying on the top stack to be able to have a single stack frame at all.

We could maybe also add a "Lazy" frame for a lazy component if there's no other owner.

This is all academic though because it only affects our tests that asserts on component stacks and real apps rarely has no owner.

</Suspense>,
);
root.update(<App />);
});
} catch (e) {
error = e;
2 changes: 1 addition & 1 deletion packages/react/src/ReactOwnerStack.js
Original file line number Diff line number Diff line change
@@ -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();
}
6 changes: 2 additions & 4 deletions packages/react/src/ReactSharedInternalsClient.js
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ export type SharedStateClient = {
thrownErrors: Array<mixed>,

// 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;
6 changes: 2 additions & 4 deletions packages/react/src/ReactSharedInternalsServer.js
Original file line number Diff line number Diff line change
@@ -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;
8 changes: 4 additions & 4 deletions packages/shared/forks/consoleWithStackDev.rn.js
Original file line number Diff line number Diff line change
@@ -23,25 +23,25 @@ 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);
}
}
}

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';
8 changes: 4 additions & 4 deletions packages/shared/forks/consoleWithStackDev.www.js
Original file line number Diff line number Diff line change
@@ -18,27 +18,27 @@ 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);
}
}
}

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);