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

Fix double preload #26729

Merged
merged 2 commits into from
Apr 25, 2023
Merged

Fix double preload #26729

merged 2 commits into from
Apr 25, 2023

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Apr 25, 2023

I found a couple scenarios where preloads were issued too aggressively

  1. During SSR, if you render a new stylesheet after the preamble flushed it will flush a preload even if the resource was already preloaded
  2. During Client render, if you call ReactDOM.preload() it will only check if a preload exists in the Document before inserting a new one. It should check for an underlying resource such as a stylesheet link or script if the preload is for a recognized asset type

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 25, 2023
gnoff added 2 commits April 25, 2023 14:33
…preload existed already in the DOM rather than checking if the underlying asset was also already in the dom
@gnoff gnoff force-pushed the fix-double-preload branch from 26e291e to b3eee80 Compare April 25, 2023 21:33
@gnoff gnoff requested review from sophiebits and sebmarkbage April 25, 2023 21:34
@react-sizebot
Copy link

Comparing: f87e97a...b3eee80

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 +0.05% 163.94 kB 164.02 kB +0.06% 51.68 kB 51.71 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.05% 169.47 kB 169.56 kB +0.05% 53.33 kB 53.36 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 565.78 kB 565.96 kB +0.03% 100.03 kB 100.06 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 549.52 kB 549.69 kB +0.03% 97.22 kB 97.24 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMServer-prod.modern.js +0.27% 134.56 kB 134.91 kB +0.32% 25.04 kB 25.12 kB
facebook-www/ReactDOMServer-prod.classic.js +0.26% 138.12 kB 138.48 kB +0.31% 25.75 kB 25.83 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +0.26% 139.39 kB 139.75 kB +0.27% 26.35 kB 26.42 kB

Generated by 🚫 dangerJS against b3eee80

@gnoff gnoff merged commit ec5e9c2 into facebook:main Apr 25, 2023
@gnoff gnoff deleted the fix-double-preload branch April 25, 2023 22:10
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
I found a couple scenarios where preloads were issued too aggressively

1. During SSR, if you render a new stylesheet after the preamble flushed
it will flush a preload even if the resource was already preloaded
2. During Client render, if you call `ReactDOM.preload()` it will only
check if a preload exists in the Document before inserting a new one. It
should check for an underlying resource such as a stylesheet link or
script if the preload is for a recognized asset type
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
I found a couple scenarios where preloads were issued too aggressively

1. During SSR, if you render a new stylesheet after the preamble flushed
it will flush a preload even if the resource was already preloaded
2. During Client render, if you call `ReactDOM.preload()` it will only
check if a preload exists in the Document before inserting a new one. It
should check for an underlying resource such as a stylesheet link or
script if the preload is for a recognized asset type

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