diff --git a/.changeset/fair-weeks-turn.md b/.changeset/fair-weeks-turn.md new file mode 100644 index 0000000000..1999f7d3a1 --- /dev/null +++ b/.changeset/fair-weeks-turn.md @@ -0,0 +1,8 @@ +--- +'@shopify/hydrogen': patch +--- + +## `` 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. diff --git a/examples/typescript/README.md b/examples/typescript/README.md index 094640a198..9636713293 100644 --- a/examples/typescript/README.md +++ b/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 ``` diff --git a/packages/hydrogen/src/components/CartLineImage/tests/CartLineImage.test.tsx b/packages/hydrogen/src/components/CartLineImage/tests/CartLineImage.test.tsx index 7fe0910043..e3ae5fcc63 100644 --- a/packages/hydrogen/src/components/CartLineImage/tests/CartLineImage.test.tsx +++ b/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(``, () => { + it('displays the image', () => { + const line = { + ...CART_LINE, + merchandise: { + ...CART_LINE.merchandise, + image: {...cartMerchandiseImage}, + }, + }; - const wrapper = mount( - - - - ); + const wrapper = mount( + + + + ); - 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( - - - - ); + const wrapper = mount( + + + + ); - 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( - - - - ); + it('displays nothing if there is no image', () => { + const wrapper = mount( + + + + ); + + 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 + ; + + // @ts-expect-error no need to pass data + ; + // @ts-expect-error no need to pass src + ; + + // @ts-expect-error foo is invalid + ; + }); }); diff --git a/packages/hydrogen/src/components/Image/Image.tsx b/packages/hydrogen/src/components/Image/Image.tsx index 4624f7f876..830c1cb008 100644 --- a/packages/hydrogen/src/components/Image/Image.tsx +++ b/packages/hydrogen/src/components/Image/Image.tsx @@ -5,16 +5,16 @@ import type {PartialDeep, Simplify, SetRequired} from 'type-fest'; type HtmlImageProps = React.ImgHTMLAttributes; -type ImageProps = Simplify< - ShopifyImageProps | ExternalImageProps ->; +type ImageProps = + | ShopifyImageProps + | ExternalImageProps; export function Image(props: ImageProps) { if (!props.data && !props.src) { throw new Error(`: requires either a 'data' or 'src' prop.`); } - if (props.data && props.src) { + if (__DEV__ && props.data && props.src) { console.warn( `: using both 'data' and 'src' props is not supported; using the 'data' prop by default` ); @@ -44,7 +44,7 @@ export type ShopifyImageProps = Omit & { * 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; + data: SetRequired, '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(`: the 'data' prop requires the 'url' property`); } - if (!data.altText && !rest.alt) { + if (__DEV__ && !data.altText && !rest.alt) { console.warn( - `: 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}` : '' - }` + `: 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( - `: the 'data' prop requires either 'width' or 'data.width', and 'height' or 'data.height' properties` + `: 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( + `: 'loader' did not return a valid string. ${`Image: ${ + data.id ?? data.url + }`}` + ); + } } /* eslint-disable hydrogen/prefer-image-component */ @@ -148,7 +157,7 @@ type LoaderProps = { }; type ExternalImageProps = 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({ ); } - if (!alt) { + if (__DEV__ && !alt) { console.warn( - `: when 'src' is provided, 'alt' should also be provided` + `: when 'src' is provided, 'alt' should also be provided. ${`Image: ${src}`}` ); } diff --git a/packages/hydrogen/src/components/Image/tests/Image.test.tsx b/packages/hydrogen/src/components/Image/tests/Image.test.tsx index 3653ecbb22..193cc79839 100644 --- a/packages/hydrogen/src/components/Image/tests/Image.test.tsx +++ b/packages/hydrogen/src/components/Image/tests/Image.test.tsx @@ -173,6 +173,7 @@ describe('', () => { width={width} height={height} loading={loading} + alt="" /> ); @@ -199,7 +200,9 @@ describe('', () => { const height = 100; expect(() => { - mount(); + mount( + + ); }).toThrowError( `: 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('', () => { const height = 0; expect(() => { - mount(); + mount( + + ); }).toThrowError( `: 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('', () => { height={height} loader={loaderMock} loaderOptions={loaderOptions} + alt="" /> ); @@ -276,4 +282,31 @@ describe('', () => { }); }); }); + + 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 + ; + ; + + // @ts-expect-error data.url + ; + + // @ts-expect-error data and src + ; + + // @ts-expect-error foo is invalid + ; + + // @ts-expect-error must have alt + ; + + // @ts-expect-error must have width + ; + + // @ts-expect-error must have height + ; + }); }); diff --git a/packages/hydrogen/src/components/MediaFile/MediaFile.tsx b/packages/hydrogen/src/components/MediaFile/MediaFile.tsx index 1bd8e42bbe..9106f205d9 100644 --- a/packages/hydrogen/src/components/MediaFile/MediaFile.tsx +++ b/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).image; - if (!dataImage) { + const dataImage = (data as PartialDeep) + .image as ShopifyImageProps['data']; + if (!dataImage || !dataImage.url) { console.warn( `No "image" property was found on the "data" prop for , for the "type='image'"` );