Generate a default srcset for an image returned by the Shopify CDN

[kiljaja]

Description

The Image component is only using src to load images. When an image is wider than the viewport, the user downloads more data than they need slowing down their page load. By using srcsets, the browser can determine the best size that requires the least data. This PR would auto-generate a srcset for Images using the Shopify CDN.

The default image sizes are [352, 832, 1200, 1920, 2560] these extend the defaults used by Shopify's liquid HTML image tag image_tag The user can define their custom sizes with the Image size options property widths, for example, [200, 300, 400]. The custom sizes work in a similar way to the liquid image_tag: widths: '200, 300, 400' works

/** Storefront API images with srcsets */

  {/* no custom srcset or directly using srcSet -> default img with sizes up to its max image width */}
  <Image data={image} />

  {/* custom srcset ->  img with sizes 200 and 300 */}
  <Image data={image}  loaderOptions={{widths: [200, 300]}}/>

  {/* using srcSet ->  user is using the img's srcset directly; this overwrites all of the above options */}
  <Image data={image}  srcSet="picSM.png 375w, picMD.png 700w ...." loaderOptions={{widths: [200, 300]}}/>

These improvements would benefit the requested image improvements in #1223 by adding best practices into the image by default while still allowing customization.

Additional context

The functionality, naming, and order are based on the Liquid HTML tag for Images. Link for reference image_tag.

---

Before submitting the PR, please make sure you do the following:

• Read the Contributing Guidelines
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
• Update docs in this repository according to your change
• Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

[frehner]

Getting some alignment on things and then I'll provide a full code review. Thanks for your patience!

[frehner]

Thanks! It would be great if you could also add some tests in Image.test.tsx around this feature as well.

[frehner]

ShopifyLoaderOptions is specifically for the object that gets passed to loader. In this case, I don't think we're going to pass widths to loader, so widths should probably just be its own top-level prop to Image.

[frehner]

In this case, it may be worth actually just enforcing that they're of type number to make things easier for ourselves, instead of string | number. But I could be persuaded either way on this one.

[kiljaja]

ShopifyLoaderOptions is specifically for the object that gets passed to loader. In this case, I don't think we're going to pass widths to loader, so widths should probably just be its own top-level prop to Image.

The widths prop is now a top-level prop of Image

[kiljaja]

In this case, it may be worth actually just enforcing that they're of type number to make things easier for ourselves, instead of string | number. But I could be persuaded either way on this one.

I think string representation of numbers should still be allowed since it follows the HMTL attributes types. The check handles both cases

[frehner]

This will result in different URLs for src and srcset if someone were to provide their own loader. So in this case, I think you want to remove addImageSizeParametersToUrl and instead take in the loader prop, and call loader() here instead. If that makes sense?

[kiljaja]

This will result in different URLs for src and srcset if someone were to provide their own loader. So in this case, I think you want to remove addImageSizeParametersToUrl and instead take in the loader prop, and call loader() here instead. If that makes sense?

srcsets are now generated with a loader if one is provided. otherwise, it defaults to using addImageSizeParametersToUrl

[frehner]

I love this check. 👍

[frehner]

Is this check to make it so that there isn't a srcset option that has a width wider than the set width of the Image? Would it work the same if you removed this from the if statement and still filtered out the srcset on the line below?

[kiljaja]

Is this check to make it so that there isn't a srcset option that has a width wider than the set width of the Image? Would it work the same if you removed this from the if statement and still filtered out the srcset on the line below?

Yes, that is correct, srcsets are kept within the max image width to prevent distortion and poor quality images.

It would work the same without the if check, but I opted to check because the filter method returns a new array, and I wanted to avoid using extra memory if it's not needed. This optimization is minor. The computation and memory cost is negligible. PLP or pages with multiple images would benefit from this check. I can be convinced to remove it.

[frehner]

We should probably do something similar to this for the ExternalImage component as well. We'd probably need to check for if there's a loader prop or not, too.

[kiljaja]

We should probably do something similar to this for the ExternalImage component as well. We'd probably need to check for if there's a loader prop or not, too.

It might be complicated for external images since we can't know how their CDN manages search parameters. We can explore this or have a curated list of CDN that we can support.

[frehner]

Sorry, I was trying to say that on ExternalImage we should also do something like

const finalSrcset = rest.srcSet ?? (loader && widths)? widths.map(width => loader(...)) : null

if that makes sense?

[kiljaja]

There is now a srcset generated for ExternalImage that has a loader and widths prop.

/** External images */

let finalSrcset = rest.srcSet ?? undefined;

  if (!finalSrcset && loader && widths) {
    // Height is a requirement in the LoaderProps, so  to keep the aspect ratio, we must determine the height based on the default values
    const heightToWidthRatio =
      parseInt(height as string) / parseInt(width as string);
    finalSrcset = widths
      ?.map((width) => parseInt(width as string, 10))
      ?.map(
        (width) =>
          `${loader({
            ...loaderOptions,
            src,
            width,
            height: Math.floor(width * heightToWidthRatio),
          })} ${width}w`
      )
      .join(', ');
  }

  /* eslint-disable hydrogen/prefer-image-component */
  return (
    <img
      {...rest}
      src={finalSrc}
      width={width}
      height={height}
      alt={alt ?? ''}
      loading={loading ?? 'lazy'}
      srcSet={finalSrcset}
    />
  );

[mcvinci]

Suggested change

	| `widths` | An array of pixel widths to overwrite the default generated srcset, for example, `[300, 600, 800]` |
	| `widths` | An array of pixel widths to overwrite the default generated srcset. For example, `[300, 600, 800]`. |

[kiljaja]

The suggestion was implemented but placed on a different line since the property was removed from this object.

[mcvinci]

Thanks, @ejfranco06! I think it might be worthwhile making the same update here so that it coincides with the code comment added to the widths props in Image.tsx: https://github.com/Shopify/hydrogen/pull/1330/files#diff-420d9b9daba4583b071a7254c864b6af742c2eaf8cda82aa8859701745271864R70

[kiljaja]

The updates are now included in the documentation.

[mcvinci]

Thanks, @ejfranco06! The docs changes look good to me. I'll defer to @frehner for a more comprehensive technical review.

[kiljaja]

Thanks, @ejfranco06! The docs changes look good to me. I'll defer to @frehner for a more comprehensive technical review.

Thank you for helping me improve the docs

[frehner]

Can you duplicate this test but instead use src instead of data?

[frehner]

One last test addition request and then I think this is good to go!

[kiljaja]

A test for external images using src with a loader and widths is now in the test suite

[frehner]

(sorry for the delay, was at a burst) Thanks for your work!