Fix Image typescript issues (#1371)

* Add TS test-ish things for Image's TS types

Make any console.warn only happen in dev

Fix Image's weird types issue with Simplify from type-fest

Update the readme for the TS example

* Fix dev warnings and a type issue

* add changelog

.changeset/fair-weeks-turn.md

@@ -0,0 +1,8 @@
+---
+'@shopify/hydrogen': patch
+---
+
+## `<Image/>` updates
+
+- Fixed some TypeScript type issues with Image.
+- `data.url` and `alt` are now required props in Typescript, but won't break the actual component if you don't pass them.

examples/typescript/README.md

@@ -15,6 +15,7 @@ Hydrogen is a React framework and SDK that you can use to build fast and dynamic
 
 ```bash
 npx degit Shopify/hydrogen/examples/typescript hydrogen-app
+cd hydrogen-app
 yarn
 yarn dev
 ```

packages/hydrogen/src/components/CartLineImage/tests/CartLineImage.test.tsx

@@ -12,55 +12,73 @@ const cartMerchandiseImage = {
   height: 300,
 };
 
-it('displays the image', () => {
-  const line = {
-    ...CART_LINE,
-    merchandise: {
-      ...CART_LINE.merchandise,
-      image: {...cartMerchandiseImage},
-    },
-  };
+describe(`<CartLineImage />`, () => {
+  it('displays the image', () => {
+    const line = {
+      ...CART_LINE,
+      merchandise: {
+        ...CART_LINE.merchandise,
+        image: {...cartMerchandiseImage},
+      },
+    };
 
-  const wrapper = mount(
-    <CartLineProvider line={line}>
-      <CartLineImage />
-    </CartLineProvider>
-  );
+    const wrapper = mount(
+      <CartLineProvider line={line}>
+        <CartLineImage />
+      </CartLineProvider>
+    );
 
-  expect(wrapper).toContainReactComponent(Image, {
-    data: line.merchandise.image,
+    expect(wrapper).toContainReactComponent(Image, {
+      data: line.merchandise.image,
+    });
   });
-});
 
-it('allows passthrough props', () => {
-  const line = {
-    ...CART_LINE,
-    merchandise: {
-      ...CART_LINE.merchandise,
-      image: {
-        ...cartMerchandiseImage,
+  it('allows passthrough props', () => {
+    const line = {
+      ...CART_LINE,
+      merchandise: {
+        ...CART_LINE.merchandise,
+        image: {
+          ...cartMerchandiseImage,
+        },
       },
-    },
-  };
+    };
 
-  const wrapper = mount(
-    <CartLineProvider line={line}>
-      <CartLineImage className="w-full" />
-    </CartLineProvider>
-  );
+    const wrapper = mount(
+      <CartLineProvider line={line}>
+        <CartLineImage className="w-full" />
+      </CartLineProvider>
+    );
 
-  expect(wrapper).toContainReactComponent(Image, {
-    data: line.merchandise.image,
-    className: 'w-full',
+    expect(wrapper).toContainReactComponent(Image, {
+      data: line.merchandise.image,
+      className: 'w-full',
+    });
   });
-});
 
-it('displays nothing if there is no image', () => {
-  const wrapper = mount(
-    <CartLineProvider line={CART_LINE}>
-      <CartLineImage />
-    </CartLineProvider>
-  );
+  it('displays nothing if there is no image', () => {
+    const wrapper = mount(
+      <CartLineProvider line={CART_LINE}>
+        <CartLineImage />
+      </CartLineProvider>
+    );
+
+    expect(wrapper).not.toContainReactComponent(Image);
+  });
 
-  expect(wrapper).not.toContainReactComponent(Image);
+  it.skip(`typescript types`, () => {
+    // this test is actually just using //@ts-expect-error as the assertion, and don't need to execute in order to have TS validation on them
+    // I don't love this idea, but at the moment I also don't have other great ideas for how to easily test our component TS types
+
+    // no errors in these situations
+    <CartLineImage />;
+
+    // @ts-expect-error no need to pass data
+    <CartLineImage data={{}} />;
+    // @ts-expect-error no need to pass src
+    <CartLineImage src="" />;
+
+    // @ts-expect-error foo is invalid
+    <CartLineImage data={{url: ''}} foo="bar" />;
+  });
 });

packages/hydrogen/src/components/Image/Image.tsx

@@ -5,16 +5,16 @@ import type {PartialDeep, Simplify, SetRequired} from 'type-fest';
 
 type HtmlImageProps = React.ImgHTMLAttributes<HTMLImageElement>;
 
-type ImageProps<GenericLoaderOpts> = Simplify<
-  ShopifyImageProps | ExternalImageProps<GenericLoaderOpts>
->;
+type ImageProps<GenericLoaderOpts> =
+  | ShopifyImageProps
+  | ExternalImageProps<GenericLoaderOpts>;
 
 export function Image<GenericLoaderOpts>(props: ImageProps<GenericLoaderOpts>) {
   if (!props.data && !props.src) {
     throw new Error(`<Image/>: requires either a 'data' or 'src' prop.`);
   }
 
-  if (props.data && props.src) {
+  if (__DEV__ && props.data && props.src) {
     console.warn(
       `<Image/>: using both 'data' and 'src' props is not supported; using the 'data' prop by default`
     );
@@ -44,7 +44,7 @@ export type ShopifyImageProps = Omit<HtmlImageProps, 'src'> & {
    * The `data` prop is required if `src` isn't used, but both props shouldn't be used
    * at the same time. If both `src` and `data` are passed, then `data` takes priority.
    */
-  data: PartialDeep<ImageType>;
+  data: SetRequired<PartialDeep<ImageType>, 'url'>;
   /** A custom function that generates the image URL. Parameters passed in
    * are either `ShopifyLoaderParams` if using the `data` prop, or the
    * `LoaderOptions` object that you pass to `loaderOptions`.
@@ -77,11 +77,11 @@ function ShopifyImage({
     throw new Error(`<Image/>: the 'data' prop requires the 'url' property`);
   }
 
-  if (!data.altText && !rest.alt) {
+  if (__DEV__ && !data.altText && !rest.alt) {
     console.warn(
-      `<Image/>: the 'data' prop should have the 'altText' property, or the 'alt' prop, and one of them should not be empty. ${
-        data.id ? `Image ID: ${data.id}` : ''
-      }`
+      `<Image/>: the 'data' prop should have the 'altText' property, or the 'alt' prop, and one of them should not be empty. ${`Image: ${
+        data.id ?? data.url
+      }`}`
     );
   }
 
@@ -90,9 +90,11 @@ function ShopifyImage({
     loaderOptions
   );
 
-  if (!finalWidth || !finalHeight) {
+  if ((__DEV__ && !finalWidth) || !finalHeight) {
     console.warn(
-      `<Image/>: the 'data' prop requires either 'width' or 'data.width', and 'height' or 'data.height' properties`
+      `<Image/>: the 'data' prop requires either 'width' or 'data.width', and 'height' or 'data.height' properties. ${`Image: ${
+        data.id ?? data.url
+      }`}`
     );
   }
 
@@ -105,6 +107,13 @@ function ShopifyImage({
       width: finalWidth,
       height: finalHeight,
     });
+    if (typeof finalSrc !== 'string' || !finalSrc) {
+      throw new Error(
+        `<Image/>: 'loader' did not return a valid string. ${`Image: ${
+          data.id ?? data.url
+        }`}`
+      );
+    }
   }
 
   /* eslint-disable hydrogen/prefer-image-component */
@@ -148,7 +157,7 @@ type LoaderProps<GenericLoaderOpts> = {
 };
 type ExternalImageProps<GenericLoaderOpts> = SetRequired<
   HtmlImageProps,
-  'src' | 'width' | 'height'
+  'src' | 'width' | 'height' | 'alt'
 > & {
   /** A custom function that generates the image URL. Parameters passed in
    * are either `ShopifyLoaderParams` if using the `data` prop, or the
@@ -185,9 +194,9 @@ function ExternalImage<GenericLoaderOpts>({
     );
   }
 
-  if (!alt) {
+  if (__DEV__ && !alt) {
     console.warn(
-      `<Image/>: when 'src' is provided, 'alt' should also be provided`
+      `<Image/>: when 'src' is provided, 'alt' should also be provided. ${`Image: ${src}`}`
     );
   }
 

packages/hydrogen/src/components/Image/tests/Image.test.tsx

@@ -173,6 +173,7 @@ describe('<Image />', () => {
           width={width}
           height={height}
           loading={loading}
+          alt=""
         />
       );
 
@@ -199,7 +200,9 @@ describe('<Image />', () => {
         const height = 100;
 
         expect(() => {
-          mount(<Image src={src} id={id} width={width} height={height} />);
+          mount(
+            <Image src={src} id={id} width={width} height={height} alt="" />
+          );
         }).toThrowError(
           `<Image/>: when 'src' is provided, 'width' and 'height' are required and need to be valid values (i.e. greater than zero). Provided values: 'src': ${src}, 'width': ${width}, 'height': ${height}`
         );
@@ -211,7 +214,9 @@ describe('<Image />', () => {
         const height = 0;
 
         expect(() => {
-          mount(<Image src={src} id={id} width={width} height={height} />);
+          mount(
+            <Image src={src} id={id} width={width} height={height} alt="" />
+          );
         }).toThrowError(
           `<Image/>: when 'src' is provided, 'width' and 'height' are required and need to be valid values (i.e. greater than zero). Provided values: 'src': ${src}, 'width': ${width}, 'height': ${height}`
         );
@@ -243,6 +248,7 @@ describe('<Image />', () => {
           height={height}
           loader={loaderMock}
           loaderOptions={loaderOptions}
+          alt=""
         />
       );
 
@@ -276,4 +282,31 @@ describe('<Image />', () => {
       });
     });
   });
+
+  it.skip(`typescript types`, () => {
+    // this test is actually just using //@ts-expect-error as the assertion, and don't need to execute in order to have TS validation on them
+    // I don't love this idea, but at the moment I also don't have other great ideas for how to easily test our component TS types
+
+    // no errors in these situations
+    <Image data={{url: ''}} />;
+    <Image src="" width="" height="" alt="" />;
+
+    // @ts-expect-error data.url
+    <Image data={{}} />;
+
+    // @ts-expect-error data and src
+    <Image data={{url: ''}} src="" width="" height="" />;
+
+    // @ts-expect-error foo is invalid
+    <Image data={{url: ''}} foo="bar" />;
+
+    // @ts-expect-error must have alt
+    <Image src="" width="" height="" />;
+
+    // @ts-expect-error must have width
+    <Image src="" alt="" height="" />;
+
+    // @ts-expect-error must have height
+    <Image src="" alt="" width="" />;
+  });
 });

packages/hydrogen/src/components/MediaFile/MediaFile.tsx

@@ -35,8 +35,9 @@ export function MediaFile({
 }: MediaFileProps) {
   switch (data.mediaContentType) {
     case 'IMAGE': {
-      const dataImage = (data as PartialDeep<MediaImageType>).image;
-      if (!dataImage) {
+      const dataImage = (data as PartialDeep<MediaImageType>)
+        .image as ShopifyImageProps['data'];
+      if (!dataImage || !dataImage.url) {
         console.warn(
           `No "image" property was found on the "data" prop for <MediaFile/>, for the "type='image'"`
         );