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

[Flight] Set Current Owner / Task When Calling console.error or invoking onError/onPostpone #30206

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jul 3, 2024

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:

Screenshot 2024-07-03 at 6 03 13 PM

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.

Copy link

vercel bot commented Jul 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2024 4:18pm

@react-sizebot
Copy link

react-sizebot commented Jul 3, 2024

Comparing: 15ca8b6...06948a6

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB +0.05% 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.99 kB 497.99 kB = 89.27 kB 89.27 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.81 kB 502.81 kB = 89.98 kB 89.97 kB
facebook-www/ReactDOM-prod.classic.js = 597.08 kB 597.08 kB = 105.33 kB 105.33 kB
facebook-www/ReactDOM-prod.modern.js = 571.42 kB 571.42 kB = 101.27 kB 101.27 kB
oss-experimental/react-server/cjs/react-server-flight.development.js +4.04% 92.56 kB 96.30 kB +4.16% 17.21 kB 17.92 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-server.node.development.js +3.09% 132.59 kB 136.68 kB +2.99% 24.82 kB 25.56 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.edge.development.js +3.08% 137.68 kB 141.93 kB +3.00% 25.64 kB 26.41 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.development.js +3.08% 137.83 kB 142.08 kB +3.00% 25.70 kB 26.47 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.node.unbundled.development.js +2.95% 138.96 kB 143.05 kB +2.87% 25.79 kB 26.53 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.development.js +2.94% 139.12 kB 143.21 kB +2.87% 25.85 kB 26.59 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.node.development.js +2.92% 140.06 kB 144.16 kB +2.85% 26.05 kB 26.79 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.development.js +2.92% 140.21 kB 144.30 kB +2.86% 26.10 kB 26.85 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.browser.development.js +2.77% 136.23 kB 140.01 kB +2.75% 25.39 kB 26.08 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js +2.76% 136.85 kB 140.63 kB +2.74% 25.56 kB 26.26 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server/cjs/react-server-flight.development.js +4.04% 92.56 kB 96.30 kB +4.16% 17.21 kB 17.92 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-server.node.development.js +3.09% 132.59 kB 136.68 kB +2.99% 24.82 kB 25.56 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.edge.development.js +3.08% 137.68 kB 141.93 kB +3.00% 25.64 kB 26.41 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.development.js +3.08% 137.83 kB 142.08 kB +3.00% 25.70 kB 26.47 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.node.unbundled.development.js +2.95% 138.96 kB 143.05 kB +2.87% 25.79 kB 26.53 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.development.js +2.94% 139.12 kB 143.21 kB +2.87% 25.85 kB 26.59 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.node.development.js +2.92% 140.06 kB 144.16 kB +2.85% 26.05 kB 26.79 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.development.js +2.92% 140.21 kB 144.30 kB +2.86% 26.10 kB 26.85 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.browser.development.js +2.77% 136.23 kB 140.01 kB +2.75% 25.39 kB 26.08 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js +2.76% 136.85 kB 140.63 kB +2.74% 25.56 kB 26.26 kB
oss-experimental/react-html/cjs/react-html.react-server.development.js +0.74% 487.54 kB 491.15 kB +0.62% 88.07 kB 88.62 kB
oss-experimental/react-server/cjs/react-server-flight.production.js +0.29% 59.35 kB 59.52 kB +0.27% 11.93 kB 11.96 kB
oss-stable-rc/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js = 125.12 kB 124.86 kB +0.24% 23.31 kB 23.36 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js = 125.12 kB 124.86 kB +0.24% 23.31 kB 23.36 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js = 125.12 kB 124.86 kB +0.24% 23.31 kB 23.36 kB
oss-stable-rc/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.browser.development.js = 124.50 kB 124.24 kB +0.23% 23.13 kB 23.19 kB
oss-stable-semver/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.browser.development.js = 124.50 kB 124.24 kB +0.23% 23.13 kB 23.19 kB
oss-stable/react-server-dom-turbopack/cjs/react-server-dom-turbopack-server.browser.development.js = 124.50 kB 124.24 kB +0.23% 23.13 kB 23.19 kB
oss-stable-rc/react-server/cjs/react-server-flight.development.js = 80.82 kB 80.48 kB +0.30% 15.03 kB 15.08 kB
oss-stable-semver/react-server/cjs/react-server-flight.development.js = 80.82 kB 80.48 kB +0.30% 15.03 kB 15.08 kB
oss-stable/react-server/cjs/react-server-flight.development.js = 80.82 kB 80.48 kB +0.30% 15.03 kB 15.08 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 06948a6

// This also gets sent to the client as the owner for the replaying log.
const componentDebugInfo: ReactComponentInfo = {
env: task.environmentName,
owner: task.debugOwner,
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Jul 3, 2024

Choose a reason for hiding this comment

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

Ideally we'd have the name of the component here too. That's why it shows up as <...> in the console task. There's two reasons for this:

  1. We don't track the "type". In Fizz we track the parent on ComponentStackNode which has this info. We could potentially eagerly create componentDebugInfo for client components in case they error instead of lazily.
  2. We don't have a "name" of ClientReferences atm anyway which is why the error message itself has a <... .

Comment on lines +2793 to +2795
ownerStack = ReactServer.captureOwnerStack
? ReactServer.captureOwnerStack()
: null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is for a more useful assertion failure when we run this test without the flag?

Suggested change
ownerStack = ReactServer.captureOwnerStack
? ReactServer.captureOwnerStack()
: null;
ownerStack = gate(flags => flags.enableOwnerStacks)
? ReactServer.captureOwnerStack()
: null;

so that we don't miss this when cleaning up the flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just copy paste from another case but this test is a bit weird in that it's gated on DEV and not just ignored in DEV.

This tracks it for attribution when serializing child properties.

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.
It also doesn't affect client errors - only attribution on the RSC server
itself.
It's annoying to have to remember to do this.

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 although Fizz
doesn't do that neither.

Also wrap onError and onPostpone with this context.
We only clear these to avoid replaying logs from onError on the client.

This doesn't make a difference because we're not clearing currentRequest
for console.error but it should line up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants