-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Bug: Random preloads added for images #27910
Comments
This. Tested with It would be "kinda" tolerable for the LCP image above fold (featured image or similar), but we have lots of lazy loaded images below fold which contain alternatives like: <img
{...imgProps}
/>
<noscript>
<img
alt={alt}
src={noscriptSrc}
/>
</noscript> For every |
I think this is actually now expected behaviour, after digging the source a bit I found f359f9b Though I could not find any docs/release notes that mention this which is a little odd as now you need to explicitly think about the loading strategy of every img tag you use. For me I was able to "fix" this by making sure all our img tags have either loading=lazy or fetchPriority=low when they are not to be preloaded, this gave me more or less only the LCP images preloaded. @nickluger For your case you could either add loading=lazy or fetchPriority=low to the fallback images, though depending on your target browsers you could get rid of the noscript parts these days as loading=lazy has pretty wide browser support now. Would be good to have this documented somewhere and also maybe add this to the react linter. |
Thanks for the info regarding loading=lazy, that could solve it. 🙏 If this is really a "feature", I have to say, I don't think it should be React's business to put stuff into my |
@muteor Which framework are you using? |
For me, its Remix. |
You're probably using their image component that uses |
I'm not using their image component but |
React is adding support for Suspensey "resource". For instance if you render a stylesheet and opt into React's handling of this using "precedence" then commits which contain this stylesheet will wait to commit until the sheet has loaded. Another area that we're making Suspensey is images. Lazy images indicate that they do not require suspense coordination but by default images will (in the future) block a boundary reveal until the image has loaded and been decoded. While these features are for client updates there are analogs for SSR. for stylesheets browsers already block paint until stylesheets load in the head. They don't however do this for images. React canaries now preload the first few images rendered if they are not lazy because we want to mimic the coordinated reveal of Suspensey images along with the first paint. It's not a perfect heuristic because it's not actually enforced to be coordinated but it does better align the behavior across suspensy vs lazy images. Another way you can signal to React that an image is not expected to be part of the first paint is to set the fetchPriority to "low" |
Good to know, thanks for the explanation. 👍 |
fwiw no framework used just renderToPipeableStream, in my case I was able to add low priority to a few icon images to get the preload to load reasonable candidates. Would be good to be able to opt-out of this somehow for a boundary, thinking of the case where you are waiting on component/data to load and don't really care about the image but lazy loading would be inappropriate for an above the fold larger image. Maybe even a way to modify the heuristic via some sort of callback would be useful. |
`<noscript>` scopes should be considered inert from the perspective of Fizz since we assume they'll only be used in rare and adverse circumstances. When we added preload support for img tags we did not include the noscript scope check in the opt-out for preloading. This change adds it in fixes: #27910
`<noscript>` scopes should be considered inert from the perspective of Fizz since we assume they'll only be used in rare and adverse circumstances. When we added preload support for img tags we did not include the noscript scope check in the opt-out for preloading. This change adds it in fixes: #27910 DiffTrain build for [dc6a7e0](dc6a7e0)
`<noscript>` scopes should be considered inert from the perspective of Fizz since we assume they'll only be used in rare and adverse circumstances. When we added preload support for img tags we did not include the noscript scope check in the opt-out for preloading. This change adds it in fixes: #27910
`<noscript>` scopes should be considered inert from the perspective of Fizz since we assume they'll only be used in rare and adverse circumstances. When we added preload support for img tags we did not include the noscript scope check in the opt-out for preloading. This change adds it in fixes: #27910
`<noscript>` scopes should be considered inert from the perspective of Fizz since we assume they'll only be used in rare and adverse circumstances. When we added preload support for img tags we did not include the noscript scope check in the opt-out for preloading. This change adds it in fixes: facebook#27910
React version: 18.3.0-canary-c29ca23af-20231205
Steps To Reproduce
Link to code example:
The current behavior
The expected behavior
To not insert preloads randomly.
Note that there is difference between renderToString and renderToPipeableStream, renderToString the preloads appear inside root and renderToPipeableStream they appear in head.
Sorry this is a bit of a question rather than bug report maybe, I have searched around and cannot see any mention of auto-preloading images but I assume there must be something going on that I have missed.
I was able to fix by using
18.3.0-canary-493f72b0a-20230727
.The text was updated successfully, but these errors were encountered: