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.

Sign up for GitHub

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

[Fiber] Enable Native console.createTask Stacks When Available #29223

Merged
merged 3 commits into from
May 26, 2024
Merged

[Fiber] Enable Native console.createTask Stacks When Available #29223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b10c0
Disable owner stacks from console.error when createTask is available
sebmarkbage May 22, 2024
e347e83
Run debug context in the console Task of the fiber
sebmarkbage May 22, 2024
5adda9c
Don't append Component Stack in React DevTools if we already have a n…
sebmarkbage May 26, 2024
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
7 changes: 7 additions & 0 deletions packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,10 @@ export function getStackByFiberInDevAndProd(
return '\nError generating stack: ' + x.message + '\n' + x.stack;
}
}

export function supportsNativeConsoleTasks(fiber: Fiber): boolean {
// If this Fiber supports native console.createTask then we are already running
// inside a native async stack trace if it's active - meaning the DevTools is open.
// Ideally we'd detect if this task was created while the DevTools was open or not.
return !!fiber._debugTask;
}
10 changes: 8 additions & 2 deletions packages/react-devtools-shared/src/backend/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import type {
import {format, formatWithStyles} from './utils';

import {getInternalReactConstants, getDispatcherRef} from './renderer';
import {getStackByFiberInDevAndProd} from './DevToolsFiberComponentStack';
import {
getStackByFiberInDevAndProd,
supportsNativeConsoleTasks,
} from './DevToolsFiberComponentStack';
import {consoleManagedByDevToolsDuringStrictMode} from 'react-devtools-feature-flags';
import {castBool, castBrowserTheme} from '../utils';

Expand Down Expand Up @@ -235,7 +238,10 @@ export function patch({
}
}

if (shouldAppendWarningStack) {
if (
shouldAppendWarningStack &&
!supportsNativeConsoleTasks(current)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hoxyq I disabled the appending this way to detect if we already have a native async stack. Ideally we could detect if this was actually an "active" task somehow (i.e. the Chrome DevTools was opened at the time).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for flagging. We could potentially look if RDT frontend is connected to RDT backend at this stage, which would mean that browser DevTools are opened for the extension case.

Would that work or do you need to know if browser DevTools are opened at the stage of task creation?

) {
const componentStack = getStackByFiberInDevAndProd(
workTagMap,
current,
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactCurrentFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,13 @@ export function runWithFiberInDEV<A0, A1, A2, A3, A4, T>(
const previousFiber = current;
setCurrentFiber(fiber);
try {
if (enableOwnerStacks) {
if (fiber !== null && fiber._debugTask) {
return fiber._debugTask.run(
callback.bind(null, arg0, arg1, arg2, arg3, arg4),
);
}
}
return callback(arg0, arg1, arg2, arg3, arg4);
} finally {
current = previousFiber;
Expand Down
9 changes: 8 additions & 1 deletion packages/shared/consoleWithStackDev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import ReactSharedInternals from 'shared/ReactSharedInternals';
import {enableOwnerStacks} from 'shared/ReactFeatureFlags';

let suppressWarning = false;
export function setSuppressWarning(newSuppressWarning) {
Expand Down Expand Up @@ -36,14 +37,20 @@ export function error(format, ...args) {
}
}

// eslint-disable-next-line react-internal/no-production-logging
const supportsCreateTask = __DEV__ && enableOwnerStacks && !!console.createTask;

function printWarning(level, format, args) {
// When changing this logic, you might want to also
// update consoleWithStackDev.www.js as well.
if (__DEV__) {
const isErrorLogger =
format === '%s\n\n%s\n' || format === '%o\n\n%s\n\n%s\n';

if (ReactSharedInternals.getCurrentStack) {
if (!supportsCreateTask && ReactSharedInternals.getCurrentStack) {
// We only add the current stack to the console when createTask is not supported.
// Since createTask requires DevTools to be open to work, this means that stacks
// can be lost while DevTools isn't open but we can't detect this.
Comment on lines +52 to +53
Copy link
Contributor

@hoxyq hoxyq May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this by design? Because I couldn't find any mention of this on web

Assuming that logs are buffered, we should expect that users will end up having different stack traces, depending on if browser DevTools are opened or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the DevTools are not open, then there will only be the stack for the current callsite and not the "async stack". If that bottoms out inside React then effectively you have no stack trace.

This is not really new to console.createTask per se but even the built-in async stack traces in Chrome DevTools are only active if they DevTools is open. That's because they come with a cost even when you don't have an error. So if they were always on you'd effectively just slow down every site for every Chrome user - even if they're not a developer. Even for developers it'd slow down every page and not just the ones you're working on.

It's unfortunate for React because we don't actually bother calling console.createTask in production at all. So if everyone followed our pattern, it wouldn't slow down any sites except the "development mode" ones and you wouldn't have DevTools open.

User space dialogs would still be able to show stacks since they get our new Error().stack version.

const stack = ReactSharedInternals.getCurrentStack();
if (stack !== '') {
format += '%s';
Expand Down