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

feat: add experimental responsive images config option #12378

Merged
merged 5 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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: 5 additions & 1 deletion packages/astro/client.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ declare module 'astro:assets' {
getImage: (
options: import('./dist/assets/types.js').UnresolvedImageTransform,
) => Promise<import('./dist/assets/types.js').GetImageResult>;
imageConfig: import('./dist/types/public/config.js').AstroConfig['image'];
imageConfig: import('./dist/types/public/config.js').AstroConfig['image'] &
Exclude<
import('./dist/types/public/config.js').AstroConfig['experimental']['responsiveImages'],
boolean
> & { experimentalResponsiveImages: boolean };
getConfiguredImageService: typeof import('./dist/assets/index.js').getConfiguredImageService;
inferRemoteSize: typeof import('./dist/assets/utils/index.js').inferRemoteSize;
Image: typeof import('./components/Image.astro').default;
Expand Down
7 changes: 7 additions & 0 deletions packages/astro/src/assets/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { WithRequired } from '../type-utils.js';
import type { ImageLayout } from '../types/public/index.js';
import type { VALID_INPUT_FORMATS, VALID_OUTPUT_FORMATS } from './consts.js';
import type { ImageService } from './services/service.js';

Expand Down Expand Up @@ -151,6 +152,12 @@ type ImageSharedProps<T> = T & {
* ```
*/
quality?: ImageQuality;

layout?: ImageLayout;

fit?: 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' | (string & {});

position?: string;
Comment on lines +158 to +160
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone with the shorter names for the HTML attributes for ease of use and avoidance of confusion of camel vs kebab

} & (
| {
/**
Expand Down
9 changes: 7 additions & 2 deletions packages/astro/src/assets/vite-plugin-assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,20 @@ export default function assets({ settings }: { settings: AstroSettings }): vite.
}
},
load(id) {
const { responsiveImages } = settings.config.experimental;
const experimentalConfig =
typeof responsiveImages === 'object' ? responsiveImages : undefined;
const experimentalResponsiveImages = Boolean(responsiveImages);

if (id === resolvedVirtualModuleId) {
return `
return /* ts */ `
export { getConfiguredImageService, isLocalService } from "astro/assets";
import { getImage as getImageInternal } from "astro/assets";
export { default as Image } from "astro/components/Image.astro";
export { default as Picture } from "astro/components/Picture.astro";
export { inferRemoteSize } from "astro/assets/utils/inferRemoteSize.js";

export const imageConfig = ${JSON.stringify(settings.config.image)};
export const imageConfig = ${JSON.stringify({ ...settings.config.image, experimentalResponsiveImages, ...experimentalConfig })};
// This is used by the @astrojs/node integration to locate images.
// It's unused on other platforms, but on some platforms like Netlify (and presumably also Vercel)
// new URL("dist/...") is interpreted by the bundler as a signal to include that directory
Expand Down
16 changes: 16 additions & 0 deletions packages/astro/src/core/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export const ASTRO_CONFIG_DEFAULTS = {
experimental: {
clientPrerender: false,
contentIntellisense: false,
responsiveImages: false,
},
} satisfies AstroUserConfig & { server: { open: boolean } };

Expand Down Expand Up @@ -525,6 +526,17 @@ export const AstroConfigSchema = z.object({
.boolean()
.optional()
.default(ASTRO_CONFIG_DEFAULTS.experimental.contentIntellisense),
responsiveImages: z
.union([
z.boolean(),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how updateConfig() will behave if the user sets it as a boolean, and an integration sets layout

z.object({
layout: z.enum(['responsive', 'fixed', 'full-width', 'none']).optional(),
objectFit: z.string().optional(),
objectPosition: z.string().optional(),
}),
])
.optional()
.default(ASTRO_CONFIG_DEFAULTS.experimental.responsiveImages),
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
})
.strict(
`Invalid or outdated experimental feature.\nCheck for incorrect spelling or outdated Astro version.\nSee https://docs.astro.build/en/reference/configuration-reference/#experimental-flags for a list of all current experiments.`,
Expand Down Expand Up @@ -681,6 +693,10 @@ export function createRelativeSchema(cmd: string, fileProtocolRoot: string) {
config.image.endpoint.route = prependForwardSlash(config.image.endpoint.route);
}

if (config.experimental.responsiveImages === true) {
config.experimental.responsiveImages = {};
}

return config;
})
.refine((obj) => !obj.outDir.toString().startsWith(obj.publicDir.toString()), {
Expand Down
51 changes: 51 additions & 0 deletions packages/astro/src/types/public/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1699,9 +1699,60 @@ export interface ViteUserConfig extends OriginalViteUserConfig {
* To use this feature with the Astro VS Code extension, you must also enable the `astro.content-intellisense` option in your VS Code settings. For editors using the Astro language server directly, pass the `contentIntellisense: true` initialization parameter to enable this feature.
*/
contentIntellisense?: boolean;

/**
* @docs
* @name experimental.responsiveImages
* @type {boolean}
* @default `undefined`
* @version 5.0.0
* @description
*
* Enables and configures automatic responsive image options for images in your project. Set to `true` (for no default option passed to your images) or an object with default responsive image configuration options.
*
* ```js
* {
* experimental: {
* responsiveImages: {
* layout: 'responsive',
Copy link
Member

Choose a reason for hiding this comment

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

This now changes in that you only set this to true and now you go to regular image config to configure the properties, right?

And, since we now have multiple properties (acknowledging there may still even be more to come!), when you do add the regular image config for this example, I would set at least a couple of them.

(Not meaning to overly edit while stuff is still happening, just didn't want us to forget!)

* },
* }
* ```
*
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
* Then, you can add a `layout` option to any `<Image />` component when needed to override your default configuration: `responsive`, `fixed`, `full-width`, or `none`. This attribute is required to transform your images if `responsiveImages.layout` is not configured. Images with a layout value of `undefined` or `none` will not be transformed.
*
* ```astro
* ---
* import { Image } from 'astro:assets';
* import myImage from '../assets/my_image.png';
* ---
* <Image src={myImage} alt="A description of my image." layout='fixed' />
* ```
*
*/

responsiveImages?:
Copy link
Member

Choose a reason for hiding this comment

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

should this be under image instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean as experimental.image, or at the top level?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the normal top level image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that something we do for experimental features?

Copy link
Member

Choose a reason for hiding this comment

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

We kinda have two philosophies, some people put it under experimental, some others don't. I personally think it makes the upgrading smoother when it's already where it will be, as you then don't need to move anything, but it's up to you really. It just felt a bit jarring to have both image and responsiveImages as two different options!

image in fact was top level while experimental, for instance

Copy link
Member

Choose a reason for hiding this comment

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

That works perfectly for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarah11918 what are your thoughts on this? Should the config options for the feature go inside experimental.responsiveImages, or shoudl that just be a boolean, with the config sitting in the top-level image object? I'm conflicted!

Copy link
Member

@sarah11918 sarah11918 Nov 5, 2024

Choose a reason for hiding this comment

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

I saw this discussion and was happy to let it play out. 😅

We have precedence for both! experimental.env kept all its config within the experimental config and there was a time where astro:i18n had an option outside of the experimental config.

I don't have a strong opinion -- people are going to have to update their config either way in the cases you've described: either they move layout to the image option directly OR they update image.experimentalLayout to just layout. It sort of to me makes more sense in that case to keep all the config together?

The other option is also a bit wonky: just use image.layout BUT it only does anything with the experimental flag set. THIS is the option that means people don't have to update their config when it comes out of experimental (other than remove the flag). It's also not necessarily pretty to document something in the main config that is essentially no op without the flag, and, it means sending the user to two different places in order to configure the experimental feature. BUT it's the only thing that has the true advantage of needing less updating on behalf of the user.

So, I won't fight any of these options. I can make them all work and there's no clear and obvious winner here. I won't make the final call unless someone specifically wants me to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the absence of any strong opinions on either side, I'll leave it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my mind again! I've gone for the second option

| boolean
| {
/**
* @docs
* @name responsiveImages.layout
* @default `undefined`
* @description
* The default layout type for responsive images. Can be overridden by the `layout` prop on the image component.
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
* - `responsive` - The image will scale to fit the container, maintaining its aspect ratio, but will not exceed the specified dimensions.
* - `fixed` - The image will maintain its original dimensions.
* - `full-width` - The image will scale to fit the container, maintaining its aspect ratio.
*/
layout?: ImageLayout | undefined;
objectFit?: 'contain' | 'cover' | 'fill' | 'none' | 'scale-down' | (string & {});
objectPosition?: string;
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
};
};
}

export type ImageLayout = 'responsive' | 'fixed' | 'full-width' | 'none';

/**
* Resolved Astro Config
*
Expand Down
Loading