Skip to content
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

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

Open
tomglaize opened this issue Sep 17, 2024 · 7 comments
Labels
Bug Something isn't working SEV-4

Comments

@tomglaize
Copy link

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.

@tomglaize tomglaize changed the title Image component throws TypeError with local assets during local development <Image /> component throws TypeError with imported local assets during development Sep 18, 2024
@wizardlyhel
Copy link
Contributor

wizardlyhel commented Sep 18, 2024

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
Copy link
Author

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
Copy link
Contributor

wizardlyhel commented Sep 18, 2024

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
Copy link
Author

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
Copy link
Contributor

wizardlyhel commented Sep 18, 2024

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
Copy link
Author

tomglaize commented Sep 18, 2024

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
Copy link
Contributor

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, '');
}

@wizardlyhel wizardlyhel added Bug Something isn't working SEV-4 labels Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working SEV-4
Projects
None yet
Development

No branches or pull requests

2 participants