[Fizz] Track Current debugTask and run it for onError Callbacks #30182
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
sebmarkbage
force-pushed
the
fizzconsoletaskerror
branch
from
f624267
to
0df009d
Compare
|
Comparing: cfb8945...9b29577 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
sebmarkbage
force-pushed
the
fizzconsoletaskerror
branch
from
0df009d
to
f1eff33
Compare
eps1lon
approved these changes
Jul 2, 2024
sebmarkbage
force-pushed
the
fizzconsoletaskerror
branch
from
f1eff33
to
30f3e83
Compare
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
github-actions bot
pushed a commit
that referenced
this pull request
Jul 2, 2024
Stacked on #30174. This tracks the current debugTask on the Task so that when an error is thrown we can use it to run the `onError` (and `onShellError` and `onFatalError`) callbacks within the Context of that task. Ideally it would be associated with the error object but neither console.error [nor reportError](https://crbug.com/350426235) reports this as the async stack so we have to manually restore it. That way when you inspect Fizz using node `--inspect` we show the right async stack. <img width="616" alt="Screenshot 2024-07-01 at 10 52 29 PM" src="https://github.com/facebook/react/assets/63648/db68133e-124e-4509-8241-c67160db94fc"> This is equivalent to how we track the task that created the parent server component or the Fiber: https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-reconciler/src/ReactChildFiber.js#L1985 Then use them when invoking the error callbacks: https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-reconciler/src/ReactFiberThrow.js#L104-L108 --------- Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com> DiffTrain build for [3db98c9](3db98c9)
sebmarkbage
added a commit
that referenced
this pull request
Jul 4, 2024
…ing onError/onPostpone (#30206) Stacked on #30197. This is similar to #30182 and #21610 in Fizz. Track the current owner/stack/task on the task. This tracks it for attribution when serializing child properties. This lets us provide the right owner and createTask when we console.error from inside Flight itself. This also affects the way we print those logs on the client since we need the owner and stack. Now console.errors that originate on the server gets the right stack on the client: <img width="760" alt="Screenshot 2024-07-03 at 6 03 13 PM" src="https://github.com/facebook/react/assets/63648/913300f8-f364-4e66-a19d-362e8d776c64"> Unfortunately, because we don't track the stack we never pop it so it'll keep tracking for serializing sibling properties. We rely on "children" typically being the last property in the common case anyway. However, this can lead to wrong attribution in some cases where the invalid property is a next property (without a wrapping element) and there's a previous element that doesn't. E.g. `<ClientComponent title={<div />} invalid={nonSerializable} />` would use the div as the attribution instead of ClientComponent. I also wrap all of our own console.error, onError and onPostpone in the context of the parent component. It's annoying to have to remember to do this though. We could always wrap the whole rendering in such as context but it would add more overhead since this rarely actually happens. It might make sense to track the whole current task instead to lower the overhead. That's what we do in Fizz. We'd still have to remember to restore the debug task though. I realize now Fizz doesn't do that neither so the debug task isn't wrapping the console.errors that Fizz itself logs. There's something off about that Flight and Fizz implementations don't perfectly align.
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 #30174.
This tracks the current debugTask on the Task so that when an error is thrown we can use it to run the
onError(andonShellErrorandonFatalError) callbacks within the Context of that task. Ideally it would be associated with the error object but neither console.error nor reportError reports this as the async stack so we have to manually restore it.That way when you inspect Fizz using node
--inspectwe show the right async stack.This is equivalent to how we track the task that created the parent server component or the Fiber:
react/packages/react-reconciler/src/ReactChildFiber.js
Line 1985 in 6d2a97a
Then use them when invoking the error callbacks:
react/packages/react-reconciler/src/ReactFiberThrow.js
Lines 104 to 108 in 6d2a97a