[Fizz][Float] Only flush Hoistable Elements if their containing Boundary is complete #27534
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
gnoff
force-pushed
the
committed-hoistables
branch
from
81d6691
to
0312468
Compare
gnoff
commented
Oct 17, 2023
| @@ -671,60 +671,6 @@ describe('ReactDOMFloat', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| // @gate enableFloat | |||
| it('emits resources before everything else when rendering with no head', async () => { | |||
There was a problem hiding this comment.
These tests weren't actually testing resource flushing order just hoistable order. These tests have been replaced by numerous others that actually cover the specific ordering so I've just removed them
gnoff
force-pushed
the
committed-hoistables
branch
from
0312468
to
dbef809
Compare
gnoff
force-pushed
the
committed-hoistables
branch
3 times, most recently
from
27aa27a
to
d14d9d6
Compare
…s at this point. Supporting this pattern required React to have a concept of an indeterminate component so that when a component first renders it can turn into either a ClassComponent or a FunctionComponent depending on what it returns. While this feature was deprecated and put behind a flag it is still in stable. This change remvoes the flag, removes the warnings, and removes the concept of IndeterminateComponent from the React codebase.
Hoistable elements and resources in fallbacks should not flush unless the fallback itself flushes. Previously we used a renderState-local binding to track the resources for the currently rendering boundary. Instead we now track this in the task itself and pass it to the functions that depend on it. This cleans up an unfortunate factoring that I put in before where during flushing we had to mimic the currently rendering Boundary to hoist correctly. This new factoring does the same thing but in a much clearer way. I do track the Boundary state separately from the Boundary itself on the task as a hot path optimization to avoid having to existence check the boundary in `pushStartInstance`. Conceptually tracking the boundary itself is sufficient but this eliminates an extra condition check. The implementation ended up not merging the boundary resource concept with hoistable state due to the very different nature in which these things need to hoist (one during task completion and the other during flush but only when flushing late boundaries). Given that I've gone back to resource specific naming rather than calling it BoundaryState.
gnoff
force-pushed
the
committed-hoistables
branch
from
d14d9d6
to
6518505
Compare
sebmarkbage
reviewed
Dec 11, 2023
| @@ -3658,11 +3770,12 @@ function flushSegment( | |||
| request: Request, | |||
| destination: Destination, | |||
| segment: Segment, | |||
| rootBoundary: null | SuspenseBoundary, | |||
There was a problem hiding this comment.
This feels leaky given that these can flush independently of any boundary. At least we should probably name this something much more esoteric and specific because this really means that the whole flushing mechanism is coupled to a very specific flushing strategy - which I'm not sure we will remember when we build SuspenseList etc. which will break some assumptions.
sebmarkbage
reviewed
Dec 11, 2023
| @@ -284,6 +293,11 @@ type Segment = { | |||
| textEmbedded: boolean, | |||
| }; | |||
|
|
|||
| type Hoistables = { | |||
| state: HoistableState, | |||
| fallbacks: Set<Hoistables>, | |||
There was a problem hiding this comment.
Does this really need to be a set? Wouldn't you know if it's added already?
sebmarkbage
reviewed
Dec 11, 2023
| // Next we merge the boundary Hoistables into the task Hoistables. In the process the boundary assumes | ||
| // the task Hoistables internal state so later if a child task also completes it will merge with | ||
| // the appropriate sets | ||
| mergeHoistables.call(parentHoistables, hoistables); |
There was a problem hiding this comment.
This is a common call. Shouldn't use .call deopt. Better to split it out into a separate helper function.
sebmarkbage
reviewed
Dec 11, 2023
| target: HoistableState, | ||
| source: HoistableState, | ||
| ) { | ||
| target.charset.push(...source.charset); |
There was a problem hiding this comment.
Let's not use spread since it'll get transpiled to a form that we don't know and likely worse than the alternative we'd do manually.
sebmarkbage
reviewed
Dec 12, 2023
| blockedSegment: Segment, // the segment we'll write to | ||
| blockedHoistables: Hoistables, // the hoistables we'll write to |
There was a problem hiding this comment.
Why isn't this just blockedBoundary.hoistables?
sebmarkbage
reviewed
Dec 12, 2023
| function finishedTask( | ||
| request: Request, | ||
| boundary: Root | SuspenseBoundary, | ||
| segment: null | Segment, | ||
| hoistables: Hoistables, | ||
| parentHoistables: null | Hoistables, |
There was a problem hiding this comment.
The parentHoistables is really reasoning on a distance here that I'm not really comfortable reasoning about. Especially in the resuming path where the nearest boundary isn't necessarily even reified. A resumed boundary is just skipped.
|
superceded by: #28069 |
gnoff
added a commit
that referenced
this pull request
Jan 30, 2024
…uspense Boundaries (#28069) Updates Fizz to handle Hoistables (Resources and Elements) in a way that better aligns with Suspense fallbacks 1. Hoistable Elements inside a fallback (regardless of how deep and how many additional boundaries are intermediate) will be ignored. The reasoning is fallbacks are transient and since there is not good way to clean up hoistables because they escape their Suspense container its better to not emit them in the first place. SSR fallbacks are already not full fidelity because they never hydrate so this aligns with that somewhat. 2. Hoistable stylesheets in fallbacks will only block the reveal of a parent suspense boundary if the fallback is going to flush with that completed parent suspense boundary. Previously if you rendered a stylesheet Resource inside a fallback any parent suspense boundaries that completed after the shell flushed would include that resource in the set required to resolve before the boundary reveal happens on the client. This is not a semantic change, just a performance optimization 3. preconnect and preload hoistable queues are gone, if you want to optimize resource loading you shoudl use `ReactDOM.preconnect` and `ReactDOM.preload`. `viewport` meta tags get their own queue because they need to go before any preloads since they affect the media state. In addition to those functional changes this PR also refactors the boundary resource tracking by moving it to the task rather than using function calls at the start of each render and flush. Tasks also now track whether they are a fallback task supercedes prior work here: #27534
github-actions bot
pushed a commit
that referenced
this pull request
Jan 30, 2024
…uspense Boundaries (#28069) Updates Fizz to handle Hoistables (Resources and Elements) in a way that better aligns with Suspense fallbacks 1. Hoistable Elements inside a fallback (regardless of how deep and how many additional boundaries are intermediate) will be ignored. The reasoning is fallbacks are transient and since there is not good way to clean up hoistables because they escape their Suspense container its better to not emit them in the first place. SSR fallbacks are already not full fidelity because they never hydrate so this aligns with that somewhat. 2. Hoistable stylesheets in fallbacks will only block the reveal of a parent suspense boundary if the fallback is going to flush with that completed parent suspense boundary. Previously if you rendered a stylesheet Resource inside a fallback any parent suspense boundaries that completed after the shell flushed would include that resource in the set required to resolve before the boundary reveal happens on the client. This is not a semantic change, just a performance optimization 3. preconnect and preload hoistable queues are gone, if you want to optimize resource loading you shoudl use `ReactDOM.preconnect` and `ReactDOM.preload`. `viewport` meta tags get their own queue because they need to go before any preloads since they affect the media state. In addition to those functional changes this PR also refactors the boundary resource tracking by moving it to the task rather than using function calls at the start of each render and flush. Tasks also now track whether they are a fallback task supercedes prior work here: #27534 DiffTrain build for [554fc49](554fc49)
gaearon
pushed a commit
that referenced
this pull request
Feb 3, 2024
…uspense Boundaries (#28069) Updates Fizz to handle Hoistables (Resources and Elements) in a way that better aligns with Suspense fallbacks 1. Hoistable Elements inside a fallback (regardless of how deep and how many additional boundaries are intermediate) will be ignored. The reasoning is fallbacks are transient and since there is not good way to clean up hoistables because they escape their Suspense container its better to not emit them in the first place. SSR fallbacks are already not full fidelity because they never hydrate so this aligns with that somewhat. 2. Hoistable stylesheets in fallbacks will only block the reveal of a parent suspense boundary if the fallback is going to flush with that completed parent suspense boundary. Previously if you rendered a stylesheet Resource inside a fallback any parent suspense boundaries that completed after the shell flushed would include that resource in the set required to resolve before the boundary reveal happens on the client. This is not a semantic change, just a performance optimization 3. preconnect and preload hoistable queues are gone, if you want to optimize resource loading you shoudl use `ReactDOM.preconnect` and `ReactDOM.preload`. `viewport` meta tags get their own queue because they need to go before any preloads since they affect the media state. In addition to those functional changes this PR also refactors the boundary resource tracking by moving it to the task rather than using function calls at the start of each render and flush. Tasks also now track whether they are a fallback task supercedes prior work here: #27534
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…uspense Boundaries (facebook#28069) Updates Fizz to handle Hoistables (Resources and Elements) in a way that better aligns with Suspense fallbacks 1. Hoistable Elements inside a fallback (regardless of how deep and how many additional boundaries are intermediate) will be ignored. The reasoning is fallbacks are transient and since there is not good way to clean up hoistables because they escape their Suspense container its better to not emit them in the first place. SSR fallbacks are already not full fidelity because they never hydrate so this aligns with that somewhat. 2. Hoistable stylesheets in fallbacks will only block the reveal of a parent suspense boundary if the fallback is going to flush with that completed parent suspense boundary. Previously if you rendered a stylesheet Resource inside a fallback any parent suspense boundaries that completed after the shell flushed would include that resource in the set required to resolve before the boundary reveal happens on the client. This is not a semantic change, just a performance optimization 3. preconnect and preload hoistable queues are gone, if you want to optimize resource loading you shoudl use `ReactDOM.preconnect` and `ReactDOM.preload`. `viewport` meta tags get their own queue because they need to go before any preloads since they affect the media state. In addition to those functional changes this PR also refactors the boundary resource tracking by moving it to the task rather than using function calls at the start of each render and flush. Tasks also now track whether they are a fallback task supercedes prior work here: facebook#27534
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.
Currently Stacked on: #27742
On the client Hoistable Elements act much like Host Components except they render into the document head instead of in their rendered position. On the server was previously not the case as any rendered hoistable would flush regardless of whether the boundary it was contained within was completed. This was even more egregious with fallbacks where if you rendered a hoistable in a fallback it would flush even if the suspense boundary never actually flushed the fallback because the primary content finished in time.
This change updates the flushing semantics of Hoistable Elements to bring them in line with the client.
What this means is that Hoistables are no longer an avenue to emit things like early preloads because if you do so within a Suspense Boundary the preload won't flush until the boundary completes making the preload likely not very useful. The general idea here is that Hoistables are not meant for things that have support in the imperative Float methods like
ReactDOM.preloadandReactDOM.preinit. Given this we are going to simplify the flushing queue ordering to not prioritized any particular hoistables over others.This PR however does not yet go all the way to a single queue for hoistables. First we have to deal with
charset. Our plan is to make this a config or infer it from some encoding parameter passed to the render function. This generally is a better way to model charset since you can't reasonably emit this one late, it's either up front or broken and it's tied to some other aspect of the rendering namely the encoding format (note that for web API environments this may be limited to utf-8 only since TextEncoder can't encode other formats)This PR also does not remove the privileged flushing for
viewportmeta tags. This is harder than charset b/c it is reasonably something you (or at least your meta-framework) might render and not know up front when you invoke the render method. If Hoistables have no privileged flushing queues then we need some means to support viewport from the imperative float methods since it emitting first before we flush things like image preloads is necessary for good performance.This PR also refactors the Resource tracking to follow the patterns established for task local state values. This is a cleaner implementation though not functionally different. The reason this was not originally pursued is the boundary resources at one point were entanged with the Float imperative methods so it needed to be part of the resolvable resources object during dispatch. We've moved away from that by allowing these methods to be called outside of Render and no longer associate anyting to the Boundary state itself so we can not clean up the implementation here a bit.