[Fiber] Enable Native console.createTask Stacks When Available #29223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
hoxyq
approved these changes
May 23, 2024
Comment on lines
+52
to
+53
| // 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
We rely on the native stack when console.createTask is available in the runtime.
This creates a native "owner stack" for the execution context. Any JSX created within this context will get new tasks created continuing the stack.
…ative task If the current Fiber already has a native task, then it might be in a native async task and so we already have a component stack. If it doesn't have a native task, it's because this is not DEV mode or it's an older React or React didn't detect console.createTask.
sebmarkbage
commented
May 26, 2024
| if (shouldAppendWarningStack) { | ||
| if ( | ||
| shouldAppendWarningStack && | ||
| !supportsNativeConsoleTasks(current) |
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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?
github-actions bot
pushed a commit
that referenced
this pull request
May 26, 2024
Stacked on #29206 and #29221. This disables appending owner stacks to console when `console.createTask` is available in the environment. Instead we rely on native "async" stacks that end up looking like this with source maps and ignore list enabled. <img width="673" alt="Screenshot 2024-05-22 at 4 00 27 PM" src="https://github.com/facebook/react/assets/63648/5313ed53-b298-4386-8f76-8eb85bdfbbc7"> Unfortunately Chrome requires a string name for each async stack and, worse, a suffix of `(async)` is automatically added which is very confusing since it seems like it might be an async component or something which it is not. In this case it's not so bad because it's nice to refer to the host component which otherwise doesn't have a stack frame since it's internal. However, if there were more owners here there would also be a `<Counter> (async)` which ends up being kind of duplicative. If the Chrome DevTools is not open from the start of the app, then `console.createTask` is disabled and so you lose the stack for those errors (or those parents if the devtools is opened later). Unlike our appended ones that are always added. That's unfortunate and likely to be a bit of a DX issue but it's also nice that it saves on perf in DEV mode for those cases. Framework dialogs can still surface the stack since we also track it in user space in parallel. This currently doesn't track Server Components yet. We need a more clever hack for that part in a follow up. I think I probably need to also add something to React DevTools to disable its stacks for this case too. Since it looks for stacks in the console.error and adds a stack otherwise. Since we don't add them anymore from the runtime, the DevTools adds them instead. DiffTrain build for commit ea6e059.
github-actions bot
pushed a commit
that referenced
this pull request
May 26, 2024
Stacked on #29206 and #29221. This disables appending owner stacks to console when `console.createTask` is available in the environment. Instead we rely on native "async" stacks that end up looking like this with source maps and ignore list enabled. <img width="673" alt="Screenshot 2024-05-22 at 4 00 27 PM" src="https://github.com/facebook/react/assets/63648/5313ed53-b298-4386-8f76-8eb85bdfbbc7"> Unfortunately Chrome requires a string name for each async stack and, worse, a suffix of `(async)` is automatically added which is very confusing since it seems like it might be an async component or something which it is not. In this case it's not so bad because it's nice to refer to the host component which otherwise doesn't have a stack frame since it's internal. However, if there were more owners here there would also be a `<Counter> (async)` which ends up being kind of duplicative. If the Chrome DevTools is not open from the start of the app, then `console.createTask` is disabled and so you lose the stack for those errors (or those parents if the devtools is opened later). Unlike our appended ones that are always added. That's unfortunate and likely to be a bit of a DX issue but it's also nice that it saves on perf in DEV mode for those cases. Framework dialogs can still surface the stack since we also track it in user space in parallel. This currently doesn't track Server Components yet. We need a more clever hack for that part in a follow up. I think I probably need to also add something to React DevTools to disable its stacks for this case too. Since it looks for stacks in the console.error and adds a stack otherwise. Since we don't add them anymore from the runtime, the DevTools adds them instead. DiffTrain build for [ea6e059](ea6e059)
|
We (the Chrome DevTools team) are looking into ways for pages to enable async stack traces without opening DevTools. Possible avenues are 1) dedicated (new) API on the Any preference from the react side how to enable async stacks or other ideas how to opt-in? |
hoxyq
added a commit
that referenced
this pull request
Jun 18, 2024
Full list of changes: * chore[react-devtools]: improve console arguments formatting before passing it to original console ([hoxyq](https://github.com/hoxyq) in [#29873](#29873)) * chore[react-devtools]: unify console patching and default to ansi escape symbols ([hoxyq](https://github.com/hoxyq) in [#29869](#29869)) * chore[react-devtools/backend]: remove consoleManagedByDevToolsDuringStrictMode ([hoxyq](https://github.com/hoxyq) in [#29856](#29856)) * chore[react-devtools/extensions]: make source maps url relative ([hoxyq](https://github.com/hoxyq) in [#29886](#29886)) * fix[react-devtools] divided inspecting elements between inspecting do… ([vzaidman](https://github.com/vzaidman) in [#29885](#29885)) * [Fiber] Create virtual Fiber when an error occurs during reconcilation ([sebmarkbage](https://github.com/sebmarkbage) in [#29804](#29804)) * fix[react-devtools] component badge in light mode is now not invisible ([vzaidman](https://github.com/vzaidman) in [#29852](#29852)) * Remove Warning: prefix and toString on console Arguments ([sebmarkbage](https://github.com/sebmarkbage) in [#29839](#29839)) * Add jest lint rules ([rickhanlonii](https://github.com/rickhanlonii) in [#29760](#29760)) * [Fiber] Track the Real Fiber for Key Warnings ([sebmarkbage](https://github.com/sebmarkbage) in [#29791](#29791)) * fix[react-devtools/store-test]: fork the test to represent current be… ([hoxyq](https://github.com/hoxyq) in [#29777](#29777)) * Default native inspections config false ([vzaidman](https://github.com/vzaidman) in [#29784](#29784)) * fix[react-devtools] remove native inspection button when it can't be used ([vzaidman](https://github.com/vzaidman) in [#29779](#29779)) * chore[react-devtools]: ip => internal-ip ([hoxyq](https://github.com/hoxyq) in [#29772](#29772)) * Fix #29724: `ip` dependency update for CVE-2024-29415 ([Rekl0w](https://github.com/Rekl0w) in [#29725](#29725)) * cleanup[react-devtools]: remove unused supportsProfiling flag from store config ([hoxyq](https://github.com/hoxyq) in [#29193](#29193)) * [Fiber] Enable Native console.createTask Stacks When Available ([sebmarkbage](https://github.com/sebmarkbage) in [#29223](#29223)) * Move createElement/JSX Warnings into the Renderer ([sebmarkbage](https://github.com/sebmarkbage) in [#29088](#29088)) * Set the current fiber to the source of the error during error reporting ([sebmarkbage](https://github.com/sebmarkbage) in [#29044](#29044)) * Unify ReactFiberCurrentOwner and ReactCurrentFiber ([sebmarkbage](https://github.com/sebmarkbage) in [#29038](#29038)) * Dim `console` calls on additional Effect invocations due to `StrictMode` ([eps1lon](https://github.com/eps1lon) in [#29007](#29007)) * refactor[react-devtools]: rewrite context menus ([hoxyq](https://github.com/hoxyq) in [#29049](#29049))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stacked on #29206 and #29221.
This disables appending owner stacks to console when
console.createTaskis available in the environment. Instead we rely on native "async" stacks that end up looking like this with source maps and ignore list enabled.Unfortunately Chrome requires a string name for each async stack and, worse, a suffix of
(async)is automatically added which is very confusing since it seems like it might be an async component or something which it is not.In this case it's not so bad because it's nice to refer to the host component which otherwise doesn't have a stack frame since it's internal. However, if there were more owners here there would also be a
<Counter> (async)which ends up being kind of duplicative.If the Chrome DevTools is not open from the start of the app, then
console.createTaskis disabled and so you lose the stack for those errors (or those parents if the devtools is opened later). Unlike our appended ones that are always added. That's unfortunate and likely to be a bit of a DX issue but it's also nice that it saves on perf in DEV mode for those cases. Framework dialogs can still surface the stack since we also track it in user space in parallel.This currently doesn't track Server Components yet. We need a more clever hack for that part in a follow up.
I think I probably need to also add something to React DevTools to disable its stacks for this case too. Since it looks for stacks in the console.error and adds a stack otherwise. Since we don't add them anymore from the runtime, the DevTools adds them instead.