-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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] Track postponed holes in the prerender pass #27317
Merged
Merged
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
facebook-github-bot
added
CLA Signed
React Core Team
Opened by a member of the React Core Team
labels
Aug 30, 2023
gnoff
approved these changes
Aug 30, 2023
Comment on lines
+6239
to
+6247
const b = new Stream.PassThrough(); | ||
b.setEncoding('utf8'); | ||
b.on('data', chunk => { | ||
writable.write(chunk); | ||
}); | ||
|
||
await act(() => { | ||
resumed.pipe(writable); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to pipe to b
or is the passthrough not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally do this so that it still closes the writable at the end. The purpose of b is to just exclude the close().
I renamed startWork to startRender and added a startPrerender. Later, we'll probably add a startResume. This just adds a non-null Map. This is a signal to the runtime to track postponed holes. Ideally this would be static but it's not worth the forking that would require. Especially since this is just a cost when you postpone other than a bit extra code.
Todo this we need to stash the rootFormatContext on the request. It's unfortunate that this is state that's not needed for regular rendering.
This is unfortunate that we need to add this to the render path in general but this doesn't really cost anything extra in terms of allocations since this object would live anyway so it's an extra field.
For now this only tracks inside boundaries but it's set up to handle holes in the root too. We now need a status field on the boundary because pendingTasks === 0 no longer implies that a boundary is complete.
This creates a tree that we can follow when replaying to find those holes. This flips the tree and encodes the ids at this point.
Because we don't flush the stream until later after we've returned we need to assign these points an ID early so that we can write them in the resume payload.
Since we're now assigning IDs eagerly to postponed boundaries and segments we can encode them into their serialized form eagerly. Instead of doing a second pass at the end.
sebmarkbage
force-pushed
the
prerendering
branch
from
August 31, 2023 15:27
03330f7
to
1aa8af6
Compare
sebmarkbage
force-pushed
the
prerendering
branch
from
August 31, 2023 16:05
1aa8af6
to
7733a15
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Aug 31, 2023
This is basically the implementation for the prerender pass. Instead of forking basically the whole implementation for prerender, I just add a conditional field on the request. If it's `null` it behaves like before. If it's non-`null` then instead of triggering client rendered boundaries it triggers those into a "postponed" state which is basically just a variant of "pending". It's supposed to be filled in later. It also builds up a serializable tree of which path can be followed to find the holes. This is basically a reverse `KeyPath` tree. It is unfortunate that this approach adds more code to the regular Fizz builds but in practice. It seems like this side is not going to add much code and we might instead just want to merge the builds so that it's smaller when you have `prerender` and `resume` in the same bundle - which I think will be common in practice. This just implements the prerender side, and not the resume side, which is why the tests have a TODO. That's in a follow up PR. DiffTrain build for [b70a0d7](b70a0d7)
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Sep 14, 2023
React upstream changes: - facebook/react#27374 - facebook/react#27369 - facebook/react#27372 - facebook/react#27371 - facebook/react#27370 - facebook/react#27321 - facebook/react#27368 - facebook/react#27367 - facebook/react#27366 - facebook/react#27360 - facebook/react#27361 - facebook/react#27357 - facebook/react#27359 - facebook/react#27358 - facebook/react#27330 - facebook/react#27347 - facebook/react#27307 - facebook/react#27346 - facebook/react#27342 - facebook/react#27340 - facebook/react#27328 - facebook/react#27327 - facebook/react#27325 - facebook/react#27337 - facebook/react#27336 - facebook/react#27323 - facebook/react#27320 - facebook/react#27317 - facebook/react#27318 - facebook/react#27316 - facebook/react#27313 - facebook/react#27309 - facebook/react#27302 - facebook/react#27297 - facebook/react#27295 - facebook/react#27305 - facebook/react#27215 - facebook/react#27304 - facebook/react#27067 - facebook/react#27179 - facebook/react#27278 - facebook/react#27277 - facebook/react#27282 - facebook/react#27230 - facebook/react#27260 - facebook/react#27270 - facebook/react#27273 - facebook/react#27268 - facebook/react#27269
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Sep 14, 2023
React upstream changes: - facebook/react#27374 - facebook/react#27369 - facebook/react#27372 - facebook/react#27371 - facebook/react#27370 - facebook/react#27321 - facebook/react#27368 - facebook/react#27367 - facebook/react#27366 - facebook/react#27360 - facebook/react#27361 - facebook/react#27357 - facebook/react#27359 - facebook/react#27358 - facebook/react#27330 - facebook/react#27347 - facebook/react#27307 - facebook/react#27346 - facebook/react#27342 - facebook/react#27340 - facebook/react#27328 - facebook/react#27327 - facebook/react#27325 - facebook/react#27337 - facebook/react#27336 - facebook/react#27323 - facebook/react#27320 - facebook/react#27317 - facebook/react#27318 - facebook/react#27316 - facebook/react#27313 - facebook/react#27309 - facebook/react#27302 - facebook/react#27297 - facebook/react#27295 - facebook/react#27305 - facebook/react#27215 - facebook/react#27304 - facebook/react#27067 - facebook/react#27179 - facebook/react#27278 - facebook/react#27277 - facebook/react#27282 - facebook/react#27230 - facebook/react#27260 - facebook/react#27270 - facebook/react#27273 - facebook/react#27268 - facebook/react#27269
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Sep 14, 2023
React upstream changes: - facebook/react#27374 - facebook/react#27369 - facebook/react#27372 - facebook/react#27371 - facebook/react#27370 - facebook/react#27321 - facebook/react#27368 - facebook/react#27367 - facebook/react#27366 - facebook/react#27360 - facebook/react#27361 - facebook/react#27357 - facebook/react#27359 - facebook/react#27358 - facebook/react#27330 - facebook/react#27347 - facebook/react#27307 - facebook/react#27346 - facebook/react#27342 - facebook/react#27340 - facebook/react#27328 - facebook/react#27327 - facebook/react#27325 - facebook/react#27337 - facebook/react#27336 - facebook/react#27323 - facebook/react#27320 - facebook/react#27317 - facebook/react#27318 - facebook/react#27316 - facebook/react#27313 - facebook/react#27309 - facebook/react#27302 - facebook/react#27297 - facebook/react#27295 - facebook/react#27305 - facebook/react#27215 - facebook/react#27304 - facebook/react#27067 - facebook/react#27179 - facebook/react#27278 - facebook/react#27277 - facebook/react#27282 - facebook/react#27230 - facebook/react#27260 - facebook/react#27270 - facebook/react#27273 - facebook/react#27268 - facebook/react#27269
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Sep 14, 2023
### React upstream changes: - facebook/react#27374 - facebook/react#27369 - facebook/react#27372 - facebook/react#27371 - facebook/react#27370 - facebook/react#27321 - facebook/react#27368 - facebook/react#27367 - facebook/react#27366 - facebook/react#27360 - facebook/react#27361 - facebook/react#27357 - facebook/react#27359 - facebook/react#27358 - facebook/react#27330 - facebook/react#27347 - facebook/react#27307 - facebook/react#27346 - facebook/react#27342 - facebook/react#27340 - facebook/react#27328 - facebook/react#27327 - facebook/react#27325 - facebook/react#27337 - facebook/react#27336 - facebook/react#27323 - facebook/react#27320 - facebook/react#27317 - facebook/react#27318 - facebook/react#27316 - facebook/react#27313 - facebook/react#27309 - facebook/react#27302 - facebook/react#27297 - facebook/react#27295 - facebook/react#27305 - facebook/react#27215 - facebook/react#27304 - facebook/react#27067 - facebook/react#27179 - facebook/react#27278 - facebook/react#27277 - facebook/react#27282 - facebook/react#27230 - facebook/react#27260 - facebook/react#27270 - facebook/react#27273 - facebook/react#27268 - facebook/react#27269
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This is basically the implementation for the prerender pass. Instead of forking basically the whole implementation for prerender, I just add a conditional field on the request. If it's `null` it behaves like before. If it's non-`null` then instead of triggering client rendered boundaries it triggers those into a "postponed" state which is basically just a variant of "pending". It's supposed to be filled in later. It also builds up a serializable tree of which path can be followed to find the holes. This is basically a reverse `KeyPath` tree. It is unfortunate that this approach adds more code to the regular Fizz builds but in practice. It seems like this side is not going to add much code and we might instead just want to merge the builds so that it's smaller when you have `prerender` and `resume` in the same bundle - which I think will be common in practice. This just implements the prerender side, and not the resume side, which is why the tests have a TODO. That's in a follow up PR.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This is basically the implementation for the prerender pass. Instead of forking basically the whole implementation for prerender, I just add a conditional field on the request. If it's `null` it behaves like before. If it's non-`null` then instead of triggering client rendered boundaries it triggers those into a "postponed" state which is basically just a variant of "pending". It's supposed to be filled in later. It also builds up a serializable tree of which path can be followed to find the holes. This is basically a reverse `KeyPath` tree. It is unfortunate that this approach adds more code to the regular Fizz builds but in practice. It seems like this side is not going to add much code and we might instead just want to merge the builds so that it's smaller when you have `prerender` and `resume` in the same bundle - which I think will be common in practice. This just implements the prerender side, and not the resume side, which is why the tests have a TODO. That's in a follow up PR. DiffTrain build for commit b70a0d7.
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.
This is basically the implementation for the prerender pass.
Instead of forking basically the whole implementation for prerender, I just add a conditional field on the request. If it's
null
it behaves like before. If it's non-null
then instead of triggering client rendered boundaries it triggers those into a "postponed" state which is basically just a variant of "pending". It's supposed to be filled in later.It also builds up a serializable tree of which path can be followed to find the holes. This is basically a reverse
KeyPath
tree.It is unfortunate that this approach adds more code to the regular Fizz builds but in practice. It seems like this side is not going to add much code and we might instead just want to merge the builds so that it's smaller when you have
prerender
andresume
in the same bundle - which I think will be common in practice.This just implements the prerender side, and not the resume side, which is why the tests have a TODO. That's in a follow up PR.