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

[Float][Fiber] Assume stylesheets in document are already loaded (cherrypick #29811) #29835

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Jun 10, 2024

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.

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.
Copy link

vercel bot commented Jun 10, 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 Jun 10, 2024 6:46pm

@gnoff gnoff merged commit 6230622 into facebook:sync-nextjs Jun 10, 2024
3 of 30 checks passed
@gnoff gnoff deleted the sync-nextjs branch June 10, 2024 18:41
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Jun 10, 2024
@react-sizebot
Copy link

Comparing: 1df34bd...a81096d

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 +0.11% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.04% 497.26 kB 497.46 kB +0.06% 89.11 kB 89.17 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.04% 502.08 kB 502.28 kB +0.06% 89.80 kB 89.85 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 594.56 kB 594.75 kB +0.06% 104.72 kB 104.78 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 570.95 kB 571.14 kB +0.06% 101.13 kB 101.19 kB
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 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-dom/unstable_server-external-runtime.js +0.96% 8.60 kB 8.69 kB +1.30% 2.23 kB 2.26 kB
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 kB 0.00 kB

Generated by 🚫 dangerJS against a81096d

ztanner added a commit to vercel/next.js that referenced this pull request Jun 11, 2024
<details>
<summary>React upstream changes</summary>

- facebook/react#29835

</details>
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.

3 participants