Skip to content

Create a root task for every Flight response #29673

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

Merged
merged 1 commit into from
May 31, 2024

Conversation

sebmarkbage
Copy link
Collaborator

This lets any element created from the server, to bottom out with a client "owner" which is the creator of the Flight request. This could be a Server Action being invoked or a router.

This is similar to how a client element bottoms out in the creator of the root element without an owner. E.g. where the root app element was created.

Without this, we inherit the task of whatever is currently executing when we're parsing which can be misleading.

Before:
Screenshot 2024-05-30 at 12 06 57 PM

After:
Screenshot 2024-05-30 at 4 59 04 PM

The before/after doesn't show much of a difference here but that's just because our Flight parsing loop is an async, which maybe it shouldn't be because it can be unnecessarily deep, and it creates a hidden line for every loop. That's what the Promise.then is. If the element is lazily initialized it's worse because we can end up in an unrelated render task as the owner - although that's its own problem.

This lets any element created from the server, to bottom out with a client
"owner" which is the creator of the Flight request. This could be a
Server Action being invoked or a router.

This is similar to how a client element bottoms out in the creator of
the root element without an owner. E.g. where the root app element was
created.

Without this, we inherit the task of whatever is currently executing when
we're parsing which can be misleading.
@sebmarkbage sebmarkbage requested a review from hoxyq May 30, 2024 21:04
Copy link

vercel bot commented May 30, 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 May 30, 2024 9:07pm

@react-sizebot
Copy link

Comparing: 9710853...1d69c7b

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.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 496.39 kB 496.39 kB = 88.84 kB 88.84 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 501.21 kB 501.21 kB = 89.54 kB 89.54 kB
facebook-www/ReactDOM-prod.classic.js = 593.88 kB 593.88 kB = 104.46 kB 104.46 kB
facebook-www/ReactDOM-prod.modern.js = 570.26 kB 570.26 kB = 100.87 kB 100.87 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 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-client/cjs/react-client-flight.development.js +0.93% 100.72 kB 101.66 kB +0.89% 22.82 kB 23.02 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.93% 100.73 kB 101.67 kB +0.93% 22.47 kB 22.67 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.93% 100.97 kB 101.91 kB +0.91% 22.53 kB 22.74 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.90% 104.22 kB 105.16 kB +0.87% 23.50 kB 23.71 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.90% 104.73 kB 105.67 kB +0.87% 23.67 kB 23.87 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.86% 108.64 kB 109.58 kB +0.83% 24.61 kB 24.82 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.85% 110.43 kB 111.37 kB +0.81% 25.18 kB 25.38 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.85% 110.46 kB 111.40 kB +0.81% 25.20 kB 25.40 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.84% 111.86 kB 112.80 kB +0.80% 25.56 kB 25.77 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.84% 111.89 kB 112.83 kB +0.79% 25.61 kB 25.81 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.83% 112.94 kB 113.88 kB +0.80% 25.78 kB 25.98 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.83% 112.96 kB 113.90 kB +0.80% 25.82 kB 26.02 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Generated by 🚫 dangerJS against 1d69c7b

@unstubbable
Copy link
Collaborator

I like that we now see the client/server boundaries in both directions! 👍 If not for this improvement, I would have suggested as an alternative to #29669, and similar to #28403, that we maybe prefix the server component tasks with componentInfo.env, e.g. Server: <App>. But this here is even better, I think. (Although it doesn't support multiple server envs, I guess.)

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented May 30, 2024

If I ignore list the bootstrapping scripts then the "use server" disappears because it gets sandwiched by two ignored things.

Screenshot 2024-05-30 at 5 10 39 PM

Ignore listing the sourceURL doesn't seem to work but it'll probably work with source maps. If I ignore list it server side the <App> entry gets dropped too:

Screenshot 2024-05-30 at 5 19 35 PM

This is more like what it'd look like in Next.js unless the element is rooted in a Server Action.

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-05-30 at 5 19 35 PM

This is more like what it'd look like in Next.js unless the element is rooted in a Server Action.

This sounds like an argument for prefixing? Especially on larger stacks it is easy to miss the "use client" and "use server" doors. But then it may be too chatty with prefixing. It doesn't block this PR and we probably just need some feedback from field usage anyway.

@sebmarkbage
Copy link
Collaborator Author

The environment name is a little tricky regardless because the way we want to set it up, it can change in the middle. So the environment name associated with a Server Component is really the environment that it entered with but it can change half way through and different stack frames in the middle. Which is likely not what someone would assume. Especially if it changes immediately inside and 99% of the code is in the other env.

One way would be to annotate the stack frames rather than the tasks but that's tricky with source maps because the name should actually be picked up from the source map in that case and not the running fake function.

@sebmarkbage sebmarkbage merged commit 8bc81ca into facebook:main May 31, 2024
github-actions bot pushed a commit that referenced this pull request May 31, 2024
This lets any element created from the server, to bottom out with a
client "owner" which is the creator of the Flight request. This could be
a Server Action being invoked or a router.

This is similar to how a client element bottoms out in the creator of
the root element without an owner. E.g. where the root app element was
created.

Without this, we inherit the task of whatever is currently executing
when we're parsing which can be misleading.

Before:
<img width="507" alt="Screenshot 2024-05-30 at 12 06 57 PM"
src="https://github.com/facebook/react/assets/63648/e234db7e-67f7-404c-958a-5c5500ffdf1f">

After:
<img width="555" alt="Screenshot 2024-05-30 at 4 59 04 PM"
src="https://github.com/facebook/react/assets/63648/8ba6acb4-2ffd-49d4-bd44-08228ad4200e">

The before/after doesn't show much of a difference here but that's just
because our Flight parsing loop is an async, which maybe it shouldn't be
because it can be unnecessarily deep, and it creates a hidden line for
every loop. That's what the `Promise.then` is. If the element is lazily
initialized it's worse because we can end up in an unrelated render task
as the owner - although that's its own problem.

DiffTrain build for commit 8bc81ca.
github-actions bot pushed a commit that referenced this pull request May 31, 2024
This lets any element created from the server, to bottom out with a
client "owner" which is the creator of the Flight request. This could be
a Server Action being invoked or a router.

This is similar to how a client element bottoms out in the creator of
the root element without an owner. E.g. where the root app element was
created.

Without this, we inherit the task of whatever is currently executing
when we're parsing which can be misleading.

Before:
<img width="507" alt="Screenshot 2024-05-30 at 12 06 57 PM"
src="https://github.com/facebook/react/assets/63648/e234db7e-67f7-404c-958a-5c5500ffdf1f">

After:
<img width="555" alt="Screenshot 2024-05-30 at 4 59 04 PM"
src="https://github.com/facebook/react/assets/63648/8ba6acb4-2ffd-49d4-bd44-08228ad4200e">

The before/after doesn't show much of a difference here but that's just
because our Flight parsing loop is an async, which maybe it shouldn't be
because it can be unnecessarily deep, and it creates a hidden line for
every loop. That's what the `Promise.then` is. If the element is lazily
initialized it's worse because we can end up in an unrelated render task
as the owner - although that's its own problem.

DiffTrain build for [8bc81ca](8bc81ca)
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.

5 participants