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

[Fizz] Postponing in the shell #27569

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

sebmarkbage
Copy link
Collaborator

When we postpone a prerender in the shell, we should just leave an empty prelude and resume from the root. While preserving any options passed in.

Since we haven't flushed anything we can't assume we've already emitted html/body tags or any resources tracked in the resumable state. This introduces a resetResumableState function to reset anything we didn't flush.

This is a bit hacky. Ideally, we probably shouldn't have tracked it as already happened until it flushed or something like that.

Basically, it's like restarting the prerender with the same options and then immediately aborting. When we add the preload headers, we'd track those as preload() being emitted after the reset and so they get readded to the resumable state in that case.

@sebmarkbage sebmarkbage requested a review from gnoff October 23, 2023 18:45
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 23, 2023
@react-sizebot
Copy link

Comparing: 6db7f42...758a018

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.min.js = 175.02 kB 175.02 kB = 54.46 kB 54.46 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.15 kB 177.15 kB = 55.14 kB 55.14 kB
facebook-www/ReactDOM-prod.classic.js = 567.58 kB 567.58 kB = 99.92 kB 99.92 kB
facebook-www/ReactDOM-prod.modern.js = 551.43 kB 551.43 kB = 97.02 kB 97.02 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.production.min.js +1.21% 37.42 kB 37.87 kB +0.86% 11.69 kB 11.79 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.07% 3.08 kB 3.11 kB +1.47% 1.15 kB 1.17 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.07% 3.08 kB 3.11 kB +1.47% 1.15 kB 1.17 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.07% 3.08 kB 3.11 kB +1.47% 1.15 kB 1.17 kB
oss-stable-semver/react-server/cjs/react-server.production.min.js +1.03% 34.67 kB 35.03 kB +0.70% 10.98 kB 11.06 kB
oss-stable/react-server/cjs/react-server.production.min.js +1.03% 34.67 kB 35.03 kB +0.70% 10.98 kB 11.06 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +0.85% 79.21 kB 79.88 kB +0.81% 24.30 kB 24.50 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +0.85% 79.34 kB 80.01 kB +0.75% 24.69 kB 24.88 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.80% 82.75 kB 83.41 kB +0.82% 25.22 kB 25.43 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.production.min.js +0.79% 84.52 kB 85.19 kB +0.75% 25.99 kB 26.19 kB
oss-experimental/react-server/cjs/react-server.development.js +0.70% 199.47 kB 200.86 kB +0.49% 46.30 kB 46.53 kB
oss-stable-semver/react-server/cjs/react-server.development.js +0.63% 189.41 kB 190.60 kB +0.41% 44.46 kB 44.64 kB
oss-stable/react-server/cjs/react-server.development.js +0.63% 189.41 kB 190.60 kB +0.41% 44.46 kB 44.64 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.development.js +0.63% 6.22 kB 6.26 kB +1.06% 1.70 kB 1.72 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-server.development.js +0.63% 6.22 kB 6.26 kB +1.06% 1.70 kB 1.72 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.development.js +0.63% 6.22 kB 6.26 kB +1.06% 1.70 kB 1.72 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.49% 408.68 kB 410.69 kB +0.42% 89.90 kB 90.27 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.49% 411.12 kB 413.13 kB +0.41% 91.01 kB 91.38 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.development.js +0.49% 411.53 kB 413.54 kB +0.40% 91.13 kB 91.50 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.49% 430.57 kB 432.67 kB +0.43% 91.96 kB 92.36 kB

Generated by 🚫 dangerJS against 758a018

Comment on lines +2515 to +2524
if (boundary === null) {
segment.id = request.nextSegmentId++;
trackedPostpones.rootSlots = segment.id;
if (request.completedRootSegment !== null) {
// Postpone the root if this was a deeper segment.
request.completedRootSegment.status = POSTPONED;
}
return;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a reference below to

throw new Error(
  'It should not be possible to possible to postpone at the root...
 )

This whole boundaryKeyPath existence check can go away or if you're concerned it could still happen in maybe we just make the message clear it isn't about postponing at the root

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Oct 23, 2023

Choose a reason for hiding this comment

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

The "root" in that case means the very first React node before entering the first element.

I'm not sure if that's actually impossible. I think it might be if you pass in a React.lazy at the root which then postpones in the reject.

But not really related to postponing in the "shell".

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ty

@sebmarkbage sebmarkbage merged commit 05fbd1a into facebook:main Oct 23, 2023
2 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 23, 2023
When we postpone a prerender in the shell, we should just leave an empty
prelude and resume from the root. While preserving any options passed
in.

Since we haven't flushed anything we can't assume we've already emitted
html/body tags or any resources tracked in the resumable state. This
introduces a resetResumableState function to reset anything we didn't
flush.

This is a bit hacky. Ideally, we probably shouldn't have tracked it as
already happened until it flushed or something like that.

Basically, it's like restarting the prerender with the same options and
then immediately aborting. When we add the preload headers, we'd track
those as preload() being emitted after the reset and so they get readded
to the resumable state in that case.

DiffTrain build for [05fbd1a](05fbd1a)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
When we postpone a prerender in the shell, we should just leave an empty
prelude and resume from the root. While preserving any options passed
in.

Since we haven't flushed anything we can't assume we've already emitted
html/body tags or any resources tracked in the resumable state. This
introduces a resetResumableState function to reset anything we didn't
flush.

This is a bit hacky. Ideally, we probably shouldn't have tracked it as
already happened until it flushed or something like that.

Basically, it's like restarting the prerender with the same options and
then immediately aborting. When we add the preload headers, we'd track
those as preload() being emitted after the reset and so they get readded
to the resumable state in that case.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
When we postpone a prerender in the shell, we should just leave an empty
prelude and resume from the root. While preserving any options passed
in.

Since we haven't flushed anything we can't assume we've already emitted
html/body tags or any resources tracked in the resumable state. This
introduces a resetResumableState function to reset anything we didn't
flush.

This is a bit hacky. Ideally, we probably shouldn't have tracked it as
already happened until it flushed or something like that.

Basically, it's like restarting the prerender with the same options and
then immediately aborting. When we add the preload headers, we'd track
those as preload() being emitted after the reset and so they get readded
to the resumable state in that case.

DiffTrain build for commit 05fbd1a.
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