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.

Sign up for GitHub

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

Refactor Props of Image and Picture component to support type checking #5788

Merged
merged 8 commits into from
Jan 18, 2023
Merged

Refactor Props of Image and Picture component to support type checking #5788

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a912cbd
correct props type
MoustaphaDev Jan 7, 2023
8f9311a
refactor Picture and Image typings
MoustaphaDev Jan 7, 2023
d90ef3d
add missing `alt` property
MoustaphaDev Jan 7, 2023
8e65030
add changeset
MoustaphaDev Jan 7, 2023
76438d4
apply suggestions
MoustaphaDev Jan 9, 2023
0b1f0f3
correct `astro/types` import
MoustaphaDev Jan 12, 2023
8bdf53d
apply suggestions
MoustaphaDev Jan 12, 2023
c245aca
convert to type import
MoustaphaDev Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/mean-suits-cover.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@astrojs/image': patch
---

- Refactor types to support props auto-completion for the `Image` and `Picture` components.
- Pass previously missing `alt` prop to the `getPicture` function
24 changes: 3 additions & 21 deletions packages/integrations/image/components/Image.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,11 @@
// @ts-ignore
import { getImage } from '../dist/index.js';
import { warnForMissingAlt } from './index.js';
import type { ImgHTMLAttributes } from './index.js';
import type { ImageMetadata, TransformOptions, OutputFormat } from '../dist/index.js';
import type { ImageComponentLocalImageProps, ImageComponentRemoteImageProps } from './index.js';

interface LocalImageProps
extends Omit<TransformOptions, 'src'>,
Copy link
Member Author

@MoustaphaDev MoustaphaDev Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, when generics are used directly inside the astro component, its props type become Record<string, any>. When extracted to another file, it works as expected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our language server doesn't support generics at the moment, but it will soon. Your workaround seems valid, though!

cc @Princesseuh for a TS review 😄

Omit<ImgHTMLAttributes, 'src' | 'width' | 'height'> {
src: ImageMetadata | Promise<{ default: ImageMetadata }>;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
}

interface RemoteImageProps extends TransformOptions, astroHTML.JSX.ImgHTMLAttributes {
src: string;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
format?: OutputFormat;
width: number;
height: number;
}

export type Props = LocalImageProps | RemoteImageProps;
export type Props = ImageComponentLocalImageProps | ImageComponentRemoteImageProps;

const { loading = 'lazy', decoding = 'async', ...props } = Astro.props as Props;
const { loading = 'lazy', decoding = 'async', ...props } = Astro.props;

if (props.alt === undefined || props.alt === null) {
warnForMissingAlt();
Expand Down
34 changes: 4 additions & 30 deletions packages/integrations/image/components/Picture.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,9 @@
---
import { getPicture } from '../dist/index.js';
import { warnForMissingAlt } from './index.js';
import type { ImgHTMLAttributes, HTMLAttributes } from './index.js';
import type { ImageMetadata, OutputFormat, TransformOptions } from '../dist/index.js';
import type { PictureComponentLocalImageProps, PictureComponentRemoteImageProps } from './index.js';

interface LocalImageProps
extends Omit<HTMLAttributes, 'src' | 'width' | 'height'>,
Omit<TransformOptions, 'src'>,
Pick<astroHTML.JSX.ImgHTMLAttributes, 'loading' | 'decoding'> {
src: ImageMetadata | Promise<{ default: ImageMetadata }>;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
sizes: HTMLImageElement['sizes'];
widths: number[];
formats?: OutputFormat[];
}

interface RemoteImageProps
extends Omit<HTMLAttributes, 'src' | 'width' | 'height'>,
TransformOptions,
Pick<ImgHTMLAttributes, 'loading' | 'decoding'> {
src: string;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
sizes: HTMLImageElement['sizes'];
widths: number[];
aspectRatio: TransformOptions['aspectRatio'];
formats?: OutputFormat[];
background: TransformOptions['background'];
}

export type Props = LocalImageProps | RemoteImageProps;
export type Props = PictureComponentLocalImageProps | PictureComponentRemoteImageProps;

const {
src,
Expand All @@ -45,7 +18,7 @@ const {
loading = 'lazy',
decoding = 'async',
...attrs
} = Astro.props as Props;
} = Astro.props;

if (alt === undefined || alt === null) {
warnForMissingAlt();
Expand All @@ -59,6 +32,7 @@ const { image, sources } = await getPicture({
fit,
background,
position,
alt,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ommiting alt was a mistake I guess?

});

delete image.width;
Expand Down
59 changes: 50 additions & 9 deletions packages/integrations/image/components/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,57 @@
/// <reference types="astro/astro-jsx" />
export { default as Image } from './Image.astro';
export { default as Picture } from './Picture.astro';
import type { HTMLAttributes as AllHTMLAttributes } from '../../../astro/types';
MoustaphaDev marked this conversation as resolved.
Show resolved Hide resolved

// TODO: should these directives be removed from astroHTML.JSX?
export type ImgHTMLAttributes = Omit<
astroHTML.JSX.ImgHTMLAttributes,
'client:list' | 'set:text' | 'set:html' | 'is:raw'
>;
export type HTMLAttributes = Omit<
astroHTML.JSX.HTMLAttributes,
'client:list' | 'set:text' | 'set:html' | 'is:raw'
>;
import type { TransformOptions, OutputFormat } from '../dist/loaders/index.js';
import type { ImageMetadata } from '../dist/vite-plugin-astro-image.js';

export interface ImageComponentLocalImageProps
extends Omit<TransformOptions, 'src'>,
Omit<ImgHTMLAttributes, 'src' | 'width' | 'height'> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be astroHTML.JSX.ImgHTMLAttributes too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe HTMLAttributes<'img'> from astro/types?

Copy link
Member Author

@MoustaphaDev MoustaphaDev Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe HTMLAttributes<'img'> from astro/types?

Sounds good to me!

Copy link
Member Author

@MoustaphaDev MoustaphaDev Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be astroHTML.JSX.ImgHTMLAttributes too?

Probably not? I don't think we want astro directives here

src: ImageMetadata | Promise<{ default: ImageMetadata }>;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
}

export interface ImageComponentRemoteImageProps extends TransformOptions, ImgHTMLAttributes {
src: string;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
format?: OutputFormat;
width: number;
height: number;
}

export interface PictureComponentLocalImageProps
extends Omit<HTMLAttributes, 'src' | 'width' | 'height'>,
Omit<TransformOptions, 'src'>,
Pick<ImgHTMLAttributes, 'loading' | 'decoding'> {
src: ImageMetadata | Promise<{ default: ImageMetadata }>;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
sizes: HTMLImageElement['sizes'];
widths: number[];
formats?: OutputFormat[];
}

export interface PictureComponentRemoteImageProps
extends Omit<HTMLAttributes, 'src' | 'width' | 'height'>,
Princesseuh marked this conversation as resolved.
Show resolved Hide resolved
TransformOptions,
Pick<ImgHTMLAttributes, 'loading' | 'decoding'> {
src: string;
/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). */
alt: string;
sizes: HTMLImageElement['sizes'];
widths: number[];
aspectRatio: TransformOptions['aspectRatio'];
formats?: OutputFormat[];
background: TransformOptions['background'];
}

export type ImgHTMLAttributes = AllHTMLAttributes<'img'>;

export type HTMLAttributes = Omit<astroHTML.JSX.HTMLAttributes, 'set:text' | 'set:html' | 'is:raw'>;
Princesseuh marked this conversation as resolved.
Show resolved Hide resolved

let altWarningShown = false;

Expand Down
2 changes: 1 addition & 1 deletion packages/integrations/image/test/fixtures/basic-image/src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const publicImage = new URL('./hero.jpg', Astro.url);
<!-- Head Stuff -->
</head>
<body>
<Image id="hero" src={publicImage.pathname} width={768} height={414} format="webp" alt="hero" />
<Image id="hero" src={publicImage.pathname} width={768} height={414} format="webp" alt="hero" />
<br />
<Image id="spaces" src={introJpg} width={768} height={414} format="webp" alt="spaces" />
<br />
Expand Down