Shopify / hydrogen

Hydrogen lets you build faster headless storefronts in less time, on Shopify.
https://hydrogen.shop
MIT License
1.44k stars 274 forks source link

`<Image />` component throws `TypeError` with imported local assets during development #2540

Closed tomglaize closed 1 month ago

tomglaize commented 1 month ago

What is the location of your example repository?

https://github.com/tomglaize/hydrogen-local-image

Which package or tool is having this issue?

Hydrogen

What version of that package or tool are you using?

2024.7.6

What version of Remix are you using?

2.10.1

Steps to Reproduce

  1. Import an image in a route or component
  2. Render the image using the <Image /> component
  3. Start Vite development server

Expected Behavior

See the image on the page. This was the behaviour prior to Vite.

Actual Behavior

The default shopifyLoader used by <Image /> throws a TypeError:

TypeError: Invalid URL string.
    at shopifyLoader (/hydrogen-local-image/node_modules/.pnpm/@shopify+hydrogen-react@2024.7.4_@types+react@18.3.7_react-dom@18.3.1_react@18.3.1/node_modules/@shopify/hydrogen-react/dist/browser-dev/Image.mjs:557:15)
    at /hydrogen-local-image/node_modules/.pnpm/@shopify+hydrogen-react@2024.7.4_@types+react@18.3.7_react-dom@18.3.1_react@18.3.1/node_modules/@shopify/hydrogen-react/dist/browser-dev/Image.mjs:495:19
    at Object.useMemo (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:9328:27)
    at Object.useMemo (/hydrogen-local-image/node_modules/.vite/deps_ssr/chunk-B7VAMJ3U.js:1094:29)
    at /hydrogen-local-image/node_modules/.pnpm/@shopify+hydrogen-react@2024.7.4_@types+react@18.3.7_react-dom@18.3.1_react@18.3.1/node_modules/@shopify/hydrogen-react/dist/browser-dev/Image.mjs:478:25
    at renderWithHooks (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:9748:24)
    at renderForwardRef (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:9896:26)
    at renderElement (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:10009:17)
    at renderNodeDestructiveImpl (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:10077:17)
    at renderNodeDestructive (/hydrogen-local-image/node_modules/.vite/deps_ssr/react-dom_server.js:10058:22)

This is because the asset url created by the import is just a path (e.g. /app/assets/image.png), which causes new URL() to throw a TypeError because it's missing a protocol and host. Before Vite, asset imports were replaced with fully formed mini-oxygen urls during development.

Note this only happens during local development. On deployment to Oxygen, the behaviour is as expected.

wizardlyhel commented 1 month ago

You should be able to override the loader with <Image> component.

// export type LoaderParams = {
//   /** The base URL of the image */
//   src?: ImageType['url'];
//   /** The URL param that controls width */
//   width?: number;
//   /** The URL param that controls height */
//   height?: number;
//   /** The URL param that controls the cropping region */
//   crop?: Crop;
// };

<Image loader={({src}: LoaderParams) => (src)} src={YOUR_LOCAL_ASSET} />
tomglaize commented 1 month ago

That would work if we could determine the full asset URL from the imported path, which is just the path to the file relative to the root of the project. However, Vite hashes the filenames of static assets (import myImg from '~/assets/my_img.jpg' resolves to /app/assets/my_img.jpg, but Vite serves that at http://localhost:3000/assets/my_img-<vite_hash>.jpg) so that would require a Vite plugin.

wizardlyhel commented 1 month ago

It works for me ..

Image file in app/assets/test.png

// In a route

import testImage from '~/assets/test.png';

// In component

export default function Homepage() {
  console.log(testImage);     // Outputs `/app/assets/test.png`
  return (
    <div className="home">
      <Image loader={({src}) => src || ''} src={testImage} alt="Test" />
    </div>
  );
}

Localhost Image

Preview Image

tomglaize commented 1 month ago

Sorry, I thought you were suggesting we write a custom loader that creates a fully formed URL for the asset and then calls the default shopifyLoader with that URL. If you just bypass shopifyLoader then yes, of course you will also avoid the part of the loader that was throwing an error. As you can see from the srcset in your screenshot, you'd then also need to re-implement the shopifyLoader behaviour if you want dynamic resizing.

The point of raising this issue was to highlight that 1) there's a difference between dev time and deployment behaviour, 2) this difference wasn't there before Vite, and 3) considering that loading static assets is a perfectly normal use case, the dev time behaviour seems like a bug. It makes sense to me that the default shopifyLoader would address this use case (e.g. by checking import.meta.env.DEV and checking if the src provided has a protocol), rather than forcing users to re-implement the default loader behaviour just to be able to load static assets.

wizardlyhel commented 1 month ago

But static assets from project won't get the same width/height/crop treatments even if you have those query parameters.

Only image assets served from https://cdn.shopify.com will be able to consume the query parameters for ?width=100&height=100&crop=center

It's why we had the loader to be overridable. Since there are devs require images from other source (other image service) that may have different url construction requirement to reconstruct the url in the proper format.

It would have been better off to just use the image tag for static assets since each srcset defined is pulling in the exact same file, despite the query parameters

tomglaize commented 1 month ago

But static assets from project won't get the same width/height/crop treatments even if you have those query parameters.

Only image assets served from https://cdn.shopify.com will be able to consume the query parameters for ?width=100&height=100&crop=center

After deployment, static assets are served from https://cdn.shopify.com and can be transformed with query params. Here's an example from our live site - the first image on the page is a static asset served from https://cdn.shopify.com/oxygen-v2/1587/16485/34015/900696/build/_assets/glaize_collage-Z3PT7B4U.webp?width=2000&height=1066&crop=center

The above is a pre-Vite deployment, but it's the same for a Vite preview deployment I just did - static assets are served from Shopify's CDN.

wizardlyhel commented 1 month ago

Ah that makes sense now - I totally forgot that static assets also got mapped from Oxygen.

Most likely a small fix for the shopifyLoader where it has a default base uri where we remove in the end.

const PLACEHOLDER_DOMAIN = 'https://placeholder.shopify.com';
export function shopifyLoader({src, width, height, crop}: LoaderParams) {
  if (!src) {
    return '';
  }

  const url = new URL(src, PLACEHOLDER_DOMAIN);

  if (width) {
    url.searchParams.append('width', Math.round(width).toString());
  }

  if (height) {
    url.searchParams.append('height', Math.round(height).toString());
  }

  if (crop) {
    url.searchParams.append('crop', crop);
  }
  return url.href.replace(PLACEHOLDER_DOMAIN, '');
}