[Float][Fiber] Assume stylesheets in document are already loaded #29811
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 20841f9...1158394 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gnoff
force-pushed
the
bugfix-stylesheet-loaded-heuristic
branch
from
00dbe6a
to
dbfbd56
Compare
sebmarkbage
reviewed
Jun 7, 2024
| @@ -138,4 +134,9 @@ export function completeBoundaryWithStyles( | |||
| ); | |||
| } | |||
|
|
|||
| function cleanupWith(cb) { | |||
There was a problem hiding this comment.
You can't declare helpers like this because they end up being globals. Do it inline instead.
sebmarkbage
reviewed
Jun 7, 2024
| @@ -139,3 +135,8 @@ export function completeBoundaryWithStyles( | |||
| ), | |||
| ); | |||
| } | |||
|
|
|||
| function cleanupWith(cb) { | |||
sebmarkbage
reviewed
Jun 7, 2024
| @@ -2601,26 +2614,21 @@ function preloadStylesheet( | |||
| ) { | |||
| preloadPropsMap.set(key, preloadProps); | |||
|
|
|||
| if (!ownerDocument.querySelector(getStylesheetSelectorFromKey(key))) { | |||
There was a problem hiding this comment.
Why is this check removed? If there is an existing style sheet why would we preload it?
There was a problem hiding this comment.
We only call this function in one place and I changed it to gate the call on the instance not existing so we don't need to duplicate the check here
gnoff
force-pushed
the
bugfix-stylesheet-loaded-heuristic
branch
from
dbfbd56
to
cf49d3a
Compare
sebmarkbage
reviewed
Jun 7, 2024
| @@ -82,18 +87,14 @@ export function completeBoundaryWithStyles( | |||
| resourceEl.setAttribute(attr, stylesheetDescriptor[j++]); | |||
| } | |||
| loadingState = resourceEl['_p'] = new Promise((resolve, reject) => { | |||
| resourceEl.onload = resolve; | |||
| resourceEl.onerror = reject; | |||
| resourceEl.onload = cleanupWith.bind(resourceEl, resolve); | |||
There was a problem hiding this comment.
It might be smaller to just inline the function like an arrow function. We probably compile them out but if we ever stop.
There was a problem hiding this comment.
need bind b/c i mutate the resourceEl the binding. not doing so leads to closure spitting out very large code b/c it gives the variable some enormous loop name
sebmarkbage
reviewed
Jun 7, 2024
| @@ -47,6 +47,11 @@ export function completeBoundaryWithStyles( | |||
| const dependencies = []; | |||
| let href, precedence, attr, loadingState, resourceEl, media; | |||
|
|
|||
| function cleanupWith(cb) { | |||
| delete this['_p']; | |||
There was a problem hiding this comment.
This should probably be this._p = null since deleting forces a dictionary mode on plain JS objects . I'm not sure if that's true for DOM nodes but I'm still scared of delete.
gnoff
force-pushed
the
bugfix-stylesheet-loaded-heuristic
branch
from
cf49d3a
to
881f80e
Compare
sebmarkbage
approved these changes
Jun 7, 2024
When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already.
gnoff
force-pushed
the
bugfix-stylesheet-loaded-heuristic
branch
from
881f80e
to
1158394
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Jun 7, 2024
) When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already. DiffTrain build for commit 20b6f4c.
github-actions bot
pushed a commit
that referenced
this pull request
Jun 7, 2024
) When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already. DiffTrain build for [20b6f4c](20b6f4c)
gnoff
added a commit
to gnoff/react
that referenced
this pull request
Jun 8, 2024
…ebook#29811) When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already.
gnoff
added a commit
that referenced
this pull request
Jun 8, 2024
) (#29812) Cherry pick of #29811 When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already.
gnoff
added a commit
that referenced
this pull request
Jun 10, 2024
…rrypick #29811) (#29835) Cherrypick #29811 When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already.
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.
When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen.
This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted.
One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already.