-
Notifications
You must be signed in to change notification settings - Fork 280
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
Images Preloading #2061
base: main
Are you sure you want to change the base?
Images Preloading #2061
Conversation
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
For the sake of brainstorming, here's a different idea that we could test: What about using the Link header for preloading instead? I think it would be easier to integrate, especially when using headers in Remix' Single Fetch: export async function loader({request, response, context}: LoaderFunctionArgs) {
// ...other code
+ response.headers.set('Link', genPreloadImageLinkHeader({url: product.selectedVariant.image}))
return {product,variants};
} Or maybe the utility direclty gets the export async function loader({request, response, context}: LoaderFunctionArgs) {
// ...other code
+ addPreloadImageLinkHeader(response, [{url: product.selectedVariant.image}, ...])
return {product, variants};
} Therefore, no need to return the image from the loader and use it in a separate function. There's also a (probably very) minor perf improvement, since the browser can see the preloading links before parsing the HTML. Also, I think this would make it work not only for SSR, but for sub-sequent navigations when Remix returns loader data with their custom JSON format... since it's a header now it should be added independently from the content-type. Which means that it would also start preloading images on link hover. -- But this is just theory, I haven't tested if this works 🤔 Any downsides to this? 🤔 |
Interesting take @frandiox I initially tried using the Even if it did support sourcesets, I'd be worried it might break the response because it would be easy to exceed the header limits as the srcsets we are defaulting to are pretty extensive (see resulting |
Yeah, I imagine the src set headers could get huge. I wonder though, is the extra loader prop necessary? Why not just: + import {genPreloadImageLinkMeta} from '@shopify/hydrogen'
export const meta: MetaFunction<typeof loader> = ({data}) => {
const metas = [
{title: `Hydrogen | ${data?.product.title ?? ''}`},
] as MetaDescriptor[];
+ const preloadImageLink = genPreloadImageLinkMeta({
+ url: data.product.selectedVariant.image,
+ });
+ metas.push(preloadImageLink);
return metas;
};
export async function loader({params, request, context}: LoaderFunctionArgs) {
// ...other code
return defer({
product,
variants,
});
} I think the pros are:
The only value for a generic prop coming off all the loaders IMO is if there was a |
Oh I completely missed the
That would sound like a good compromise so that the whole thing only happens in 1 place 🤔 Related to my comment though:
I guess then there's no way to preload images for sub-sequent navigations? 😞 |
Great point! Did the extra loader return because I was initially going to write two proposals one including a at the root, but then I then reverted back considering we just deprecated the component in favor of using straight up meta. I will update the implementation to eliminate the extra return from the loader. |
I was trying to think through this at the burst, and AFAIK there's no way to do it. Unless we simplify the image loading to not use |
I actually think it's fine if it doesn't include Just putting it out there! |
…for 3p loader function
I don't think this would work. If you preload an image format/size that doesn't get rendered then it actually has detrimental impact to performance and the browser will warn about it. Preloading a placeholder seems to me like deop. -- I've now updated the readme with an instruction to the collection route and removed the returned |
Let's also think about how would React 19 would change all this with document metadata hoisting https://react.dev/blog/2024/04/25/react-19#support-for-metadata-tags |
Hydrogen Image Preloading
Preloading above-the-fold content has a powerful effect on Largest Contentful Paint (LCP). Specially when it comes to fonts and images, as both images and text nodes can be LCP candidates. Hero images and large runs of text that are rendered using web fonts can benefit significantly from a well-placed preload hint, and should be used when there are opportunities to deliver these important bits of content to the user faster.
Current situation
Hydrogen currently does not offer a native abstraction, example or guide on how to effectively implement image preloading. Although Remix offers all the right primitives, developers have to go out of their way to recognize the importance and implementation details of preloading critical resources to improve performance.
preload.mp4
👎🏼 Main homepage hero image not preloaded
👍🏼 Main homepage hero image preloaded in the homepage
Example generated tag for skeleton's home route.
Proposal
The follow driven-development doc, demonstrates a proposal to facilitate images preloading.
Note
A separate proposal will be made for handling web fonts and other critical resources
Code Changes
If we prefer to keep this as userland feature we should export hydrogen-react
Image
component inner utilities within@shopify/hydrogen
generateImageWidths
generateSizes
generateSrcSet
srcSet
for Shopify images based on an array of sizesshopifyLoader
Otherwise, we can export the
genPreloadImageLinkMeta
from hydrogen which internally uses the Image utilitiesImplementation
On each applicable route, import the
genPreloadImageLinkMeta
utility and call it if apreload
prop is presentin the
loader
returnPreloading product featured image
Preloading the first 4 product images in collections
Additional Reading