[Hydrogen React] Don't generate srcset if size is above source image dimensions

[andershagbard]

WHY are these changes introduced?

Fixes #2456

WHAT is this pull request doing?

• Prevent <Image /> from generating a srcset image with higher dimensions than source image. This is to prevent the Shopify CDN from just using the original image uncropped.
• Minor changes to functions
• Minor grammar corrections
• Major bump because of changes in exported function generateSrcSet

Only downside is when not using aspectRatio. In case the image is 750px wide, the srcset goes from 600w to 800w, and will not generate an image for 800w with 750px width. Not sure if we should still generate it?

HOW to test your changes?

Tests has been added.

Checklist

• I've read the Contributing Guidelines
• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've added a changeset if this PR contains user-facing or noteworthy changes
• I've added tests to cover my changes
• I've added or updated the documentation

[blittle]

Thank you so much for this PR! It looks good to me, other than I don't think we need it to be a breaking change because generateSrcSet is not exposed externally. Please change it to be a patch change instead.

[andershagbard]

Setting to draft. The file is exported, so someone might use it. I may have a way to go around changing the generateSrcSet

[andershagbard]

Refactored so it's a real patch change now.

[juanpprieto]

Thanks for this excellent PR 🙏🏼

[adrianxadamn]

Hi @andershagbard. I'm trying to wrap my head around why we shouldn't generate a srcset size if the srcset we provide from a srcSetOption interval exceeds the image dimensions. Instead of removing that srcset size, would it be better to apply the max width as the last src set option and break from there?

Say we use these options for srcSetOptions but the client uploaded 4 square images for a row of images in a section with: 600x, 1100x, 1100x, 1100x. The images with 1100x dimension will show 3 srcSet sizes of 300w, 700w, 1100w. However, the 1st image will only show 1 src set option of 300w. I know this is a fault from our client, but I think it would be beneficial to at least add 1 more src set option of the max potential width of the image.

srcSetOptions={
    intervals: 8,
    startingWidth: 300,
    incrementSize: 400,
    placeholderWidth: 100,
}

The Last question I have is what is the logic behind the Image component to render srcSetOptions as x descriptors instead of w descriptors. Depending on the srcSets passed for an image, sometimes I would get 1x, 2x, 3x, srcSets instead of 300w, 700w, 1100w

[andershagbard]

@adrianxadamn

I also stated this in the PR.

Only downside is when not using aspectRatio. In case the image is 750px wide, the srcset goes from 600w to 800w, and will not generate an image for 800w with 750px width. Not sure if we should still generate it?

We could potentially update the component to include the highest possible image with the assigned aspect. I don't see any downside to doing this.

The Last question I have is what is the logic behind the Image component to render srcSetOptions as x descriptors instead of w descriptors. Depending on the srcSets passed for an image, sometimes I would get 1x, 2x, 3x, srcSets instead of 300w, 700w, 1100w

I haven't touched this logic in this PR.

But if you pass a sizes attribute you will get w descriptors, and if you pass a width attribute you get 1x, 2x, 3x.

This is because the 1x, 2x, 3x produce the most precise sizes for fixed width images, and w produce the best results for fluid size images.

[adrianxadamn]

Hey @andershagbard. Thanks for getting back to me.

Yes, I would love to see the component include the highest possible image with the assigned aspect. But now that I think of it, it might seem silly to see 5000w if a client added a very large image. So maybe not the best option.

However, I did find something interesting. I know your PR did not involve any updates around what gets rendered such as w vs z descriptors. But I like to discuss things further about the hidden rules behind it. I noticed that the component will render w descriptors if more than 3 srcset options were returned, (if width in data is passed and size attribute is added). Because we are not generating src set options that exceed the image's max dimensions anymore, some images will return >= 3 src set options which return the src sets as x descriptors. If we're using my example above of passing 8 intervals with an incrementSize of 300 for a 1100x image, the image component will return srcsets as x descriptors which is not what I expected.

But now knowing this, along with the new image component updates, I need to rethink about the srcSetOptions I pass to get the srcset descriptors I'm expecting.

[andershagbard]

@adrianxadamn I would limit the biggest image to the max size defined by the srcSetOptions property.

I think in most cases, you don't have to pass a custom srcSetOptions as the default is good enough. I know that you can generate more "perfect" sizes. But if you reuse the same image, with the same srcSetOptions throughout your site, it will actually be a faster site, as the Shopify CDN will have more cache hits.