From 3d17a859a4e0d514afd64ddb070d40569256f989 Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Mon, 11 Nov 2024 13:14:37 +0000 Subject: [PATCH 1/7] wip: add crop support to image services --- packages/astro/components/Image.astro | 38 +++++++++++-------- packages/astro/src/assets/layout.ts | 6 +-- packages/astro/src/assets/services/service.ts | 18 ++++++++- packages/astro/src/assets/services/sharp.ts | 25 ++++++++++-- packages/astro/src/assets/types.ts | 7 +++- packages/astro/src/types/public/config.ts | 5 +-- packages/astro/test/core-image-layout.test.js | 2 +- 7 files changed, 70 insertions(+), 31 deletions(-) diff --git a/packages/astro/components/Image.astro b/packages/astro/components/Image.astro index b873e1ebad71..70cbc0f45602 100644 --- a/packages/astro/components/Image.astro +++ b/packages/astro/components/Image.astro @@ -23,6 +23,20 @@ if (typeof props.height === 'string') { props.height = parseInt(props.height); } +const { experimentalResponsiveImages } = imageConfig; + +const layoutClassMap = { + fixed: 'aim-fi', + responsive: 'aim-re', +}; + +if (experimentalResponsiveImages) { + // Apply defaults from imageConfig if not provided + props.layout ??= imageConfig.experimentalLayout; + props.fit ??= imageConfig.experimentalObjectFit ?? 'cover'; + props.position ??= imageConfig.experimentalObjectPosition ?? 'center'; +} + const image = await getImage(props); const additionalAttributes: HTMLAttributes<'img'> = {}; @@ -34,16 +48,7 @@ if (import.meta.env.DEV) { additionalAttributes['data-image-component'] = 'true'; } -const { experimentalResponsiveImages } = imageConfig; - -const layoutClassMap = { - fixed: 'aim-fi', - responsive: 'aim-re', -}; - const cssFitValues = ['fill', 'contain', 'cover', 'scale-down']; -const objectFit = props.fit ?? imageConfig.experimentalObjectFit ?? 'cover'; -const objectPosition = props.position ?? imageConfig.experimentalObjectPosition ?? 'center'; // The style prop can't be spread when using define:vars, so we need to extract it here // @see https://github.com/withastro/compiler/issues/1050 @@ -51,7 +56,7 @@ const { style = '', class: className, ...attrs } = { ...additionalAttributes, .. --- { - experimentalResponsiveImages ? ( + experimentalResponsiveImages && props.layout ? ( <img src={image.src} {...attrs} @@ -64,12 +69,13 @@ const { style = '', class: className, ...attrs } = { ...additionalAttributes, .. } <style - define:vars={experimentalResponsiveImages && { - w: image.attributes.width ?? props.width ?? image.options.width, - h: image.attributes.height ?? props.height ?? image.options.height, - fit: cssFitValues.includes(objectFit) && objectFit, - pos: objectPosition, - }} + define:vars={experimentalResponsiveImages && + props.layout && { + w: image.attributes.width ?? props.width ?? image.options.width, + h: image.attributes.height ?? props.height ?? image.options.height, + fit: cssFitValues.includes(props.fit) && props.fit, + pos: props.position, + }} > /* Shared by all Astro images */ .aim { diff --git a/packages/astro/src/assets/layout.ts b/packages/astro/src/assets/layout.ts index a7fabf4e1306..adc117f39967 100644 --- a/packages/astro/src/assets/layout.ts +++ b/packages/astro/src/assets/layout.ts @@ -1,4 +1,4 @@ -import type { ImageLayout } from '../types/public/index.js'; +import type { ImageLayout } from './types.js'; // Common screen widths. These will be filtered according to the image size and layout export const DEFAULT_RESOLUTIONS = [ @@ -33,9 +33,9 @@ export const LIMITED_RESOLUTIONS = [ /** * Gets the breakpoints for an image, based on the layout and width - * + * * The rules are as follows: - * + * * - For full-width layout we return all breakpoints smaller than the original image width * - For fixed layout we return 1x and 2x the requested width, unless the original image is smaller than that. * - For responsive layout we return all breakpoints smaller than 2x the requested width, unless the original image is smaller than that. diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts index 23c983b8c5ad..278e02279ab5 100644 --- a/packages/astro/src/assets/services/service.ts +++ b/packages/astro/src/assets/services/service.ts @@ -2,7 +2,12 @@ import { AstroError, AstroErrorData } from '../../core/errors/index.js'; import { isRemotePath, joinPaths } from '../../core/path.js'; import type { AstroConfig } from '../../types/public/config.js'; import { DEFAULT_HASH_PROPS, DEFAULT_OUTPUT_FORMAT, VALID_SUPPORTED_FORMATS } from '../consts.js'; -import type { ImageOutputFormat, ImageTransform, UnresolvedSrcSetValue } from '../types.js'; +import type { + ImageFit, + ImageOutputFormat, + ImageTransform, + UnresolvedSrcSetValue, +} from '../types.js'; import { isESMImportedImage } from '../utils/imageKind.js'; import { isRemoteAllowed } from '../utils/remotePattern.js'; @@ -116,6 +121,8 @@ export type BaseServiceTransform = { height?: number; format: string; quality?: string | null; + fit?: ImageFit; + position?: string; }; const sortNumeric = (a: number, b: number) => a - b; @@ -221,7 +228,10 @@ export const baseService: Omit<LocalImageService, 'transform'> = { // Sometimes users will pass number generated from division, which can result in floating point numbers if (options.width) options.width = Math.round(options.width); if (options.height) options.height = Math.round(options.height); - + if (options.layout && options.width && options.height) { + options.fit ??= 'cover'; + delete options.layout; + } return options; }, getHTMLAttributes(options) { @@ -344,6 +354,8 @@ export const baseService: Omit<LocalImageService, 'transform'> = { h: 'height', q: 'quality', f: 'format', + fit: 'fit', + position: 'position', }; Object.entries(params).forEach(([param, key]) => { @@ -366,6 +378,8 @@ export const baseService: Omit<LocalImageService, 'transform'> = { height: params.has('h') ? parseInt(params.get('h')!) : undefined, format: params.get('f') as ImageOutputFormat, quality: params.get('q'), + fit: params.get('fit') as ImageFit, + position: params.get('position') ?? undefined, }; return transform; diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index c9df4c269ac6..3398c29dbed6 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -1,6 +1,6 @@ -import type { FormatEnum, SharpOptions } from 'sharp'; +import type { FitEnum, FormatEnum, SharpOptions } from 'sharp'; import { AstroError, AstroErrorData } from '../../core/errors/index.js'; -import type { ImageOutputFormat, ImageQualityPreset } from '../types.js'; +import type { ImageFit, ImageOutputFormat, ImageQualityPreset } from '../types.js'; import { type BaseServiceTransform, type LocalImageService, @@ -38,6 +38,15 @@ async function loadSharp() { return sharpImport; } +const fitMap: Record<ImageFit, keyof FitEnum> = { + fill: 'fill', + contain: 'contain', + cover: 'cover', + none: 'outside', + 'scale-down': 'inside', + '': 'outside', +}; + const sharpService: LocalImageService<SharpImageServiceConfig> = { validateOptions: baseService.validateOptions, getURL: baseService.getURL, @@ -62,8 +71,16 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { // always call rotate to adjust for EXIF data orientation result.rotate(); - // Never resize using both width and height at the same time, prioritizing width. - if (transform.height && !transform.width) { + // If `fit` isn't set then don't use both width and height (old behavior) + if (transform.width && transform.height && transform.fit) { + const fit: keyof FitEnum = fitMap[transform.fit] ?? transform.fit ?? 'outside'; + result.resize({ + width: Math.round(transform.width), + height: Math.round(transform.height), + fit, + position: transform.position, + }); + } else if (transform.height && !transform.width) { result.resize({ height: Math.round(transform.height) }); } else if (transform.width) { result.resize({ width: Math.round(transform.width) }); diff --git a/packages/astro/src/assets/types.ts b/packages/astro/src/assets/types.ts index e4687adfdfa6..0e46cfb45117 100644 --- a/packages/astro/src/assets/types.ts +++ b/packages/astro/src/assets/types.ts @@ -1,4 +1,3 @@ -import type { ImageLayout } from '../types/public/index.js'; import type { OmitPreservingIndexSignature, Simplify, WithRequired } from '../type-utils.js'; import type { VALID_INPUT_FORMATS, VALID_OUTPUT_FORMATS } from './consts.js'; import type { ImageService } from './services/service.js'; @@ -7,6 +6,8 @@ export type ImageQualityPreset = 'low' | 'mid' | 'high' | 'max' | (string & {}); export type ImageQuality = ImageQualityPreset | number; export type ImageInputFormat = (typeof VALID_INPUT_FORMATS)[number]; export type ImageOutputFormat = (typeof VALID_OUTPUT_FORMATS)[number] | (string & {}); +export type ImageLayout = 'responsive' | 'fixed' | 'full-width' | 'none'; +export type ImageFit = 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' | (string & {}); export type AssetsGlobalStaticImagesList = Map< string, @@ -87,6 +88,8 @@ export type ImageTransform = { height?: number | undefined; quality?: ImageQuality | undefined; format?: ImageOutputFormat | undefined; + fit?: ImageFit | undefined; + position?: string | undefined; [key: string]: any; }; @@ -157,7 +160,7 @@ type ImageSharedProps<T> = T & { layout?: ImageLayout; - fit?: 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' | (string & {}); + fit?: ImageFit; position?: string; } & ( diff --git a/packages/astro/src/types/public/config.ts b/packages/astro/src/types/public/config.ts index ce4052b4370f..27d5a286a7cf 100644 --- a/packages/astro/src/types/public/config.ts +++ b/packages/astro/src/types/public/config.ts @@ -13,6 +13,7 @@ import type { REDIRECT_STATUS_CODES } from '../../core/constants.js'; import type { Logger, LoggerLevel } from '../../core/logger/core.js'; import type { EnvSchema } from '../../env/schema.js'; import type { AstroIntegration } from './integrations.js'; +import type { ImageFit, ImageLayout } from '../../assets/types.js'; export type Locales = (string | { codes: string[]; path: string })[]; @@ -1091,7 +1092,7 @@ export interface ViteUserConfig extends OriginalViteUserConfig { * The default object-fit value for responsive images. Can be overridden by the `fit` prop on the image component. * Requires the `experimental.responsiveImages` flag to be enabled. */ - experimentalObjectFit?: 'contain' | 'cover' | 'fill' | 'none' | 'scale-down' | (string & {}); + experimentalObjectFit?: ImageFit; /** * @docs * @name image.experimentalObjectPosition @@ -1766,8 +1767,6 @@ export interface ViteUserConfig extends OriginalViteUserConfig { }; } -export type ImageLayout = 'responsive' | 'fixed' | 'full-width' | 'none'; - /** * Resolved Astro Config * diff --git a/packages/astro/test/core-image-layout.test.js b/packages/astro/test/core-image-layout.test.js index 205336c0c743..09715bb5c1c3 100644 --- a/packages/astro/test/core-image-layout.test.js +++ b/packages/astro/test/core-image-layout.test.js @@ -134,7 +134,7 @@ describe('astro:image:layout', () => { assert.match(style, /\.aim\[/); assert.match(style, /\.aim-re\[/); assert.match(style, /\.aim-fi\[/); - }) + }); }); describe('srcsets', () => { From 28715dc757c83ea64848d4d6db1e0774c4b97454 Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 10:12:42 +0000 Subject: [PATCH 2/7] Add tests --- packages/astro/src/assets/services/service.ts | 3 ++ packages/astro/test/core-image-layout.test.js | 47 +++++++++++++++++++ .../core-image-layout/src/pages/fit.astro | 35 ++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts index 278e02279ab5..0aa3ac7d180c 100644 --- a/packages/astro/src/assets/services/service.ts +++ b/packages/astro/src/assets/services/service.ts @@ -232,6 +232,9 @@ export const baseService: Omit<LocalImageService, 'transform'> = { options.fit ??= 'cover'; delete options.layout; } + if(options.fit === 'none') { + delete options.fit; + } return options; }, getHTMLAttributes(options) { diff --git a/packages/astro/test/core-image-layout.test.js b/packages/astro/test/core-image-layout.test.js index 09715bb5c1c3..1341685fdef2 100644 --- a/packages/astro/test/core-image-layout.test.js +++ b/packages/astro/test/core-image-layout.test.js @@ -180,6 +180,53 @@ describe('astro:image:layout', () => { }); }); + describe('generated URLs', () => { + let $; + before(async () => { + let res = await fixture.fetch('/fit'); + let html = await res.text(); + $ = cheerio.load(html); + }); + it('generates width and height in image URLs when both are provided', () => { + let $img = $('#local-both img'); + const aspectRatio = 300 / 400; + const srcset = parseSrcset($img.attr('srcset')) + for (const { url } of srcset) { + const params = new URL(url, 'https://example.com').searchParams; + const width = parseInt(params.get('w')); + const height = parseInt(params.get('h')); + assert.equal(width / height, aspectRatio); + } + }); + + it('sets a default fit of "cover" when no fit is provided', () => { + let $img = $('#fit-default img'); + const srcset = parseSrcset($img.attr('srcset')) + for (const { url } of srcset) { + const params = new URL(url, 'https://example.com').searchParams; + assert.equal(params.get('fit'), 'cover'); + } + }) + + it('sets a fit of "contain" when fit="contain" is provided', () => { + let $img = $('#fit-contain img'); + const srcset = parseSrcset($img.attr('srcset')) + for (const { url } of srcset) { + const params = new URL(url, 'https://example.com').searchParams; + assert.equal(params.get('fit'), 'contain'); + } + }) + + it('sets no fit when fit="none" is provided', () => { + let $img = $('#fit-none img'); + const srcset = parseSrcset($img.attr('srcset')) + for (const { url } of srcset) { + const params = new URL(url, 'https://example.com').searchParams; + assert.ok(!params.has('fit')); + } + }) + }); + describe('remote images', () => { describe('srcset', () => { let $; diff --git a/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro b/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro new file mode 100644 index 000000000000..654078992556 --- /dev/null +++ b/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro @@ -0,0 +1,35 @@ +--- +import { Image } from "astro:assets"; +import penguin from "../assets/penguin.jpg"; +--- + +<div id="local-both"> + <Image src={penguin} alt="a penguin" width={300} height={400}/> +</div> +<div id="fit-default"> + <Image src={penguin} alt="a penguin" /> +</div> +<div id="fit-fill"> + <Image src={penguin} alt="a penguin" fit="fill" /> +</div> +<div id="fit-contain"> + <Image src={penguin} alt="a penguin" fit="contain" /> +</div> +<div id="fit-cover"> + <Image src={penguin} alt="a penguin" fit="cover" /> +</div> +<div id="fit-scale-down"> + <Image src={penguin} alt="a penguin" fit="scale-down" /> +</div> +<div id="fit-inside"> + <Image src={penguin} alt="a penguin" fit="inside" /> +</div> +<div id="fit-none"> + <Image src={penguin} alt="a penguin" fit="none" /> +</div> +<div id="fit-unsupported"> + <Image src={penguin} alt="a penguin" fit="unsupported" /> +</div> +<div id="position"> + <Image src={penguin} alt="a penguin" position="top left" /> +</div> From f3b65698a4f86b6db123c256fc5e71d11c14b845 Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 10:27:08 +0000 Subject: [PATCH 3/7] Strip crop attributes --- packages/astro/src/assets/services/service.ts | 2 ++ packages/astro/test/core-image-layout.test.js | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts index 0aa3ac7d180c..612cd9a3f7c4 100644 --- a/packages/astro/src/assets/services/service.ts +++ b/packages/astro/src/assets/services/service.ts @@ -250,6 +250,8 @@ export const baseService: Omit<LocalImageService, 'transform'> = { formats, layout, priority, + fit, + position, ...attributes } = options; return { diff --git a/packages/astro/test/core-image-layout.test.js b/packages/astro/test/core-image-layout.test.js index 1341685fdef2..6f42f69b8e56 100644 --- a/packages/astro/test/core-image-layout.test.js +++ b/packages/astro/test/core-image-layout.test.js @@ -199,6 +199,13 @@ describe('astro:image:layout', () => { } }); + it('does not pass through fit and position', async () => { + const fit = $('#fit-cover img'); + assert.ok(!fit.attr('fit')); + const position = $('#position img'); + assert.ok(!position.attr('position')); + }) + it('sets a default fit of "cover" when no fit is provided', () => { let $img = $('#fit-default img'); const srcset = parseSrcset($img.attr('srcset')) From 0627be4041fdaef17be32fbf79acb72d32d1785f Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 11:15:47 +0000 Subject: [PATCH 4/7] Don't upscale --- packages/astro/src/assets/services/sharp.ts | 5 +- .../astro/test/core-image-service.test.js | 134 ++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 packages/astro/test/core-image-service.test.js diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index 3398c29dbed6..aba80f4ea9d2 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -79,11 +79,12 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { height: Math.round(transform.height), fit, position: transform.position, + withoutEnlargement: true }); } else if (transform.height && !transform.width) { - result.resize({ height: Math.round(transform.height) }); + result.resize({ height: Math.round(transform.height), withoutEnlargement: Boolean(transform.fit) }); } else if (transform.width) { - result.resize({ width: Math.round(transform.width) }); + result.resize({ width: Math.round(transform.width), withoutEnlargement: Boolean(transform.fit) }); } if (transform.format) { diff --git a/packages/astro/test/core-image-service.test.js b/packages/astro/test/core-image-service.test.js new file mode 100644 index 000000000000..0c001b96cce5 --- /dev/null +++ b/packages/astro/test/core-image-service.test.js @@ -0,0 +1,134 @@ +import assert from 'node:assert/strict'; +import { after, before, describe, it } from 'node:test'; +import * as cheerio from 'cheerio'; +import { loadFixture } from './test-utils.js'; +import { lookup as probe } from '../dist/assets/utils/vendor/image-size/lookup.js'; + +async function getImageDimensionsFromFixture(fixture, path) { + /** @type { Response } */ + const res = await fixture.fetch(path instanceof URL ? path.pathname + path.search : path); + const buffer = await res.arrayBuffer(); + const { width, height } = await probe(new Uint8Array(buffer)); + return { width, height }; +} + +describe('astro image service', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + + describe('dev image service', () => { + /** @type {import('./test-utils').DevServer} */ + let devServer; + /** @type {Array<{ type: any, level: 'error', message: string; }>} */ + let logs = []; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/core-image-layout/', + image: { + domains: ['unsplash.com'], + }, + }); + + devServer = await fixture.startDevServer({}); + }); + + after(async () => { + await devServer.stop(); + }); + + describe('generated images', () => { + let $; + let src + before(async () => { + const res = await fixture.fetch('/fit'); + const html = await res.text(); + $ = cheerio.load(html); + let $img = $('#local-both img'); + src = new URL($img.attr('src'), 'http://localhost').href + }); + + it('generates width and height in image URLs when both are provided', async () => { + const url = new URL(src); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 300); + assert.equal(height, 400); + }); + + it('generates height in image URLs when only width is provided', async () => { + const url = new URL(src); + url.searchParams.delete('h'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('generates width in image URLs when only height is provided', async () => { + const url = new URL(src); + url.searchParams.delete('w'); + url.searchParams.set('h', '400'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 600); + assert.equal(height, 400); + }); + + it('preserves aspect ratio when fit=inside', async () => { + const url = new URL(src); + url.searchParams.set('fit', 'inside'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('preserves aspect ratio when fit=scale-down', async () => { + const url = new URL(src); + url.searchParams.set('fit', 'scale-down'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('preserves aspect ratio when fit=outside', async () => { + const url = new URL(src); + url.searchParams.set('fit', 'outside'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 600); + assert.equal(height, 400); + }); + const originalWidth = 2316; + const originalHeight = 1544; + it('does not upscale image if requested size is larger than original', async () => { + const url = new URL(src); + url.searchParams.set('w', '3000'); + url.searchParams.set('h', '2000'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, originalWidth); + assert.equal(height, originalHeight); + }); + + // To match old behavior, we should upscale if the requested size is larger than the original + it('does upscale image if requested size is larger than original and fit is unset', async () => { + const url = new URL(src); + url.searchParams.set('w', '3000'); + url.searchParams.set('h', '2000'); + url.searchParams.delete('fit'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, 3000); + assert.equal(height, 2000); + }) + + // To match old behavior, we should upscale if the requested size is larger than the original + it('does not upscale is only one dimension is provided and fit is set', async () => { + const url = new URL(src); + url.searchParams.set('w', '3000'); + url.searchParams.delete('h'); + url.searchParams.set('fit', 'cover'); + const { width, height } = await getImageDimensionsFromFixture(fixture, url); + assert.equal(width, originalWidth); + assert.equal(height, originalHeight); + }) + }); + + }); + +}); From 4b83ba7c6c128f68f75607679b4566771f834049 Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 12:28:17 +0000 Subject: [PATCH 5/7] Format --- packages/astro/src/assets/services/service.ts | 2 +- packages/astro/src/assets/services/sharp.ts | 19 +++++++++++++++---- packages/astro/test/core-image-layout.test.js | 16 ++++++++-------- .../astro/test/core-image-service.test.js | 12 +++++------- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts index 612cd9a3f7c4..d84ec1728e8e 100644 --- a/packages/astro/src/assets/services/service.ts +++ b/packages/astro/src/assets/services/service.ts @@ -232,7 +232,7 @@ export const baseService: Omit<LocalImageService, 'transform'> = { options.fit ??= 'cover'; delete options.layout; } - if(options.fit === 'none') { + if (options.fit === 'none') { delete options.fit; } return options; diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index aba80f4ea9d2..29b12b72fe45 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -71,7 +71,12 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { // always call rotate to adjust for EXIF data orientation result.rotate(); - // If `fit` isn't set then don't use both width and height (old behavior) + // If `fit` isn't set then use old behavior: + // - Do not use both width and height for resizing, and prioritize width over height + // - Allow enlarging images + + const withoutEnlargement = Boolean(transform.fit) && transform.fit !== 'none'; + if (transform.width && transform.height && transform.fit) { const fit: keyof FitEnum = fitMap[transform.fit] ?? transform.fit ?? 'outside'; result.resize({ @@ -79,12 +84,18 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { height: Math.round(transform.height), fit, position: transform.position, - withoutEnlargement: true + withoutEnlargement, }); } else if (transform.height && !transform.width) { - result.resize({ height: Math.round(transform.height), withoutEnlargement: Boolean(transform.fit) }); + result.resize({ + height: Math.round(transform.height), + withoutEnlargement, + }); } else if (transform.width) { - result.resize({ width: Math.round(transform.width), withoutEnlargement: Boolean(transform.fit) }); + result.resize({ + width: Math.round(transform.width), + withoutEnlargement, + }); } if (transform.format) { diff --git a/packages/astro/test/core-image-layout.test.js b/packages/astro/test/core-image-layout.test.js index 6f42f69b8e56..eb42656da082 100644 --- a/packages/astro/test/core-image-layout.test.js +++ b/packages/astro/test/core-image-layout.test.js @@ -190,7 +190,7 @@ describe('astro:image:layout', () => { it('generates width and height in image URLs when both are provided', () => { let $img = $('#local-both img'); const aspectRatio = 300 / 400; - const srcset = parseSrcset($img.attr('srcset')) + const srcset = parseSrcset($img.attr('srcset')); for (const { url } of srcset) { const params = new URL(url, 'https://example.com').searchParams; const width = parseInt(params.get('w')); @@ -204,34 +204,34 @@ describe('astro:image:layout', () => { assert.ok(!fit.attr('fit')); const position = $('#position img'); assert.ok(!position.attr('position')); - }) + }); it('sets a default fit of "cover" when no fit is provided', () => { let $img = $('#fit-default img'); - const srcset = parseSrcset($img.attr('srcset')) + const srcset = parseSrcset($img.attr('srcset')); for (const { url } of srcset) { const params = new URL(url, 'https://example.com').searchParams; assert.equal(params.get('fit'), 'cover'); } - }) + }); it('sets a fit of "contain" when fit="contain" is provided', () => { let $img = $('#fit-contain img'); - const srcset = parseSrcset($img.attr('srcset')) + const srcset = parseSrcset($img.attr('srcset')); for (const { url } of srcset) { const params = new URL(url, 'https://example.com').searchParams; assert.equal(params.get('fit'), 'contain'); } - }) + }); it('sets no fit when fit="none" is provided', () => { let $img = $('#fit-none img'); - const srcset = parseSrcset($img.attr('srcset')) + const srcset = parseSrcset($img.attr('srcset')); for (const { url } of srcset) { const params = new URL(url, 'https://example.com').searchParams; assert.ok(!params.has('fit')); } - }) + }); }); describe('remote images', () => { diff --git a/packages/astro/test/core-image-service.test.js b/packages/astro/test/core-image-service.test.js index 0c001b96cce5..d41e9386c56b 100644 --- a/packages/astro/test/core-image-service.test.js +++ b/packages/astro/test/core-image-service.test.js @@ -22,7 +22,7 @@ describe('astro image service', () => { /** @type {Array<{ type: any, level: 'error', message: string; }>} */ let logs = []; - before(async () => { + before(async () => { fixture = await loadFixture({ root: './fixtures/core-image-layout/', image: { @@ -39,13 +39,13 @@ describe('astro image service', () => { describe('generated images', () => { let $; - let src + let src; before(async () => { const res = await fixture.fetch('/fit'); const html = await res.text(); $ = cheerio.load(html); let $img = $('#local-both img'); - src = new URL($img.attr('src'), 'http://localhost').href + src = new URL($img.attr('src'), 'http://localhost').href; }); it('generates width and height in image URLs when both are provided', async () => { @@ -115,7 +115,7 @@ describe('astro image service', () => { const { width, height } = await getImageDimensionsFromFixture(fixture, url); assert.equal(width, 3000); assert.equal(height, 2000); - }) + }); // To match old behavior, we should upscale if the requested size is larger than the original it('does not upscale is only one dimension is provided and fit is set', async () => { @@ -126,9 +126,7 @@ describe('astro image service', () => { const { width, height } = await getImageDimensionsFromFixture(fixture, url); assert.equal(width, originalWidth); assert.equal(height, originalHeight); - }) + }); }); - }); - }); From dddca70286bcb4a9f8e8aa41837d7c3c46a2eb25 Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 14:09:58 +0000 Subject: [PATCH 6/7] Get build working properly --- packages/astro/src/assets/consts.ts | 2 +- packages/astro/src/assets/services/sharp.ts | 11 ++- .../astro/test/core-image-service.test.js | 88 +++++++++++++++++-- .../core-image-layout/src/pages/build.astro | 66 ++++++++++++++ .../core-image-layout/src/pages/fit.astro | 2 +- 5 files changed, 156 insertions(+), 13 deletions(-) create mode 100644 packages/astro/test/fixtures/core-image-layout/src/pages/build.astro diff --git a/packages/astro/src/assets/consts.ts b/packages/astro/src/assets/consts.ts index 15f9fe46fb24..cc9e80760702 100644 --- a/packages/astro/src/assets/consts.ts +++ b/packages/astro/src/assets/consts.ts @@ -26,4 +26,4 @@ export const VALID_SUPPORTED_FORMATS = [ ] as const; export const DEFAULT_OUTPUT_FORMAT = 'webp' as const; export const VALID_OUTPUT_FORMATS = ['avif', 'png', 'webp', 'jpeg', 'jpg', 'svg'] as const; -export const DEFAULT_HASH_PROPS = ['src', 'width', 'height', 'format', 'quality']; +export const DEFAULT_HASH_PROPS = ['src', 'width', 'height', 'format', 'quality', 'fit', 'position']; diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index 29b12b72fe45..81e5822527b4 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -40,11 +40,12 @@ async function loadSharp() { const fitMap: Record<ImageFit, keyof FitEnum> = { fill: 'fill', - contain: 'contain', + contain: 'inside', cover: 'cover', none: 'outside', 'scale-down': 'inside', - '': 'outside', + 'outside': 'outside', + 'inside': 'inside', }; const sharpService: LocalImageService<SharpImageServiceConfig> = { @@ -55,7 +56,6 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { getSrcSet: baseService.getSrcSet, async transform(inputBuffer, transformOptions, config) { if (!sharp) sharp = await loadSharp(); - const transform: BaseServiceTransform = transformOptions as BaseServiceTransform; // Return SVGs as-is @@ -75,10 +75,9 @@ const sharpService: LocalImageService<SharpImageServiceConfig> = { // - Do not use both width and height for resizing, and prioritize width over height // - Allow enlarging images - const withoutEnlargement = Boolean(transform.fit) && transform.fit !== 'none'; - + const withoutEnlargement = Boolean(transform.fit); if (transform.width && transform.height && transform.fit) { - const fit: keyof FitEnum = fitMap[transform.fit] ?? transform.fit ?? 'outside'; + const fit: keyof FitEnum = fitMap[transform.fit] ?? 'inside'; result.resize({ width: Math.round(transform.width), height: Math.round(transform.height), diff --git a/packages/astro/test/core-image-service.test.js b/packages/astro/test/core-image-service.test.js index d41e9386c56b..6e21196f69e6 100644 --- a/packages/astro/test/core-image-service.test.js +++ b/packages/astro/test/core-image-service.test.js @@ -3,6 +3,7 @@ import { after, before, describe, it } from 'node:test'; import * as cheerio from 'cheerio'; import { loadFixture } from './test-utils.js'; import { lookup as probe } from '../dist/assets/utils/vendor/image-size/lookup.js'; +import { removeDir } from '@astrojs/internal-helpers/fs'; async function getImageDimensionsFromFixture(fixture, path) { /** @type { Response } */ @@ -12,6 +13,12 @@ async function getImageDimensionsFromFixture(fixture, path) { return { width, height }; } +async function getImageDimensionsFromLocalFile(fixture, path) { + const buffer = await fixture.readFile(path, null); + const { width, height } = await probe(new Uint8Array(buffer)); + return { width, height }; +} + describe('astro image service', () => { /** @type {import('./test-utils').Fixture} */ let fixture; @@ -48,14 +55,14 @@ describe('astro image service', () => { src = new URL($img.attr('src'), 'http://localhost').href; }); - it('generates width and height in image URLs when both are provided', async () => { + it('generates correct width and height when both are provided', async () => { const url = new URL(src); const { width, height } = await getImageDimensionsFromFixture(fixture, url); assert.equal(width, 300); assert.equal(height, 400); }); - it('generates height in image URLs when only width is provided', async () => { + it('generates correct height when only width is provided', async () => { const url = new URL(src); url.searchParams.delete('h'); const { width, height } = await getImageDimensionsFromFixture(fixture, url); @@ -63,7 +70,7 @@ describe('astro image service', () => { assert.equal(height, 200); }); - it('generates width in image URLs when only height is provided', async () => { + it('generates correct width when only height is provided', async () => { const url = new URL(src); url.searchParams.delete('w'); url.searchParams.set('h', '400'); @@ -80,9 +87,9 @@ describe('astro image service', () => { assert.equal(height, 200); }); - it('preserves aspect ratio when fit=scale-down', async () => { + it('preserves aspect ratio when fit=contain', async () => { const url = new URL(src); - url.searchParams.set('fit', 'scale-down'); + url.searchParams.set('fit', 'contain'); const { width, height } = await getImageDimensionsFromFixture(fixture, url); assert.equal(width, 300); assert.equal(height, 200); @@ -129,4 +136,75 @@ describe('astro image service', () => { }); }); }); + + describe('build image service', () => { + before(async () => { + fixture = await loadFixture({ + root: './fixtures/core-image-layout/', + }); + removeDir(new URL('./fixtures/core-image-ssg/node_modules/.astro', import.meta.url)); + + await fixture.build(); + }); + + describe('generated images', () => { + let $; + let src; + before(async () => { + const html = await fixture.readFile('/build/index.html'); + $ = cheerio.load(html); + }); + + it('generates correct width and height when both are provided', async () => { + const path = $('.both img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 300); + assert.equal(height, 400); + }); + + it('generates correct height when only width is provided', async () => { + const path = $('.width-only img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('generates correct width when only height is provided', async () => { + const path = $('.height-only img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 600); + assert.equal(height, 400); + }); + + it('preserves aspect ratio when fit=inside', async () => { + const path = $('.fit-inside img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('preserves aspect ratio when fit=contain', async () => { + const path = $('.fit-contain img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 300); + assert.equal(height, 200); + }); + + it('preserves aspect ratio when fit=outside', async () => { + const path = $('.fit-outside img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, 600); + assert.equal(height, 400); + }); + const originalWidth = 2316; + const originalHeight = 1544; + it('does not upscale image if requested size is larger than original', async () => { + const path = $('.too-large img').attr('src'); + const { width, height } = await getImageDimensionsFromLocalFile(fixture, path); + assert.equal(width, originalWidth); + assert.equal(height, originalHeight); + }); + + }); + }); }); diff --git a/packages/astro/test/fixtures/core-image-layout/src/pages/build.astro b/packages/astro/test/fixtures/core-image-layout/src/pages/build.astro new file mode 100644 index 000000000000..a4a0cc9083da --- /dev/null +++ b/packages/astro/test/fixtures/core-image-layout/src/pages/build.astro @@ -0,0 +1,66 @@ +--- +import { Image } from "astro:assets"; +import penguin from "../assets/penguin.jpg"; +--- + +<div class="both"> + <Image src={penguin} alt="a penguin" width={300} height={400}/> +</div> + +<div class="width-only"> + <Image src={penguin} alt="a penguin" width={300}/> +</div> + +<div class="height-only"> + <Image src={penguin} alt="a penguin" height={400}/> +</div> + +<div class="fit-contain"> + <Image + src={penguin} + alt="a penguin" + width={300} + height={400} + fit="contain" + /> +</div> + +<div class="fit-scale-down"> + <Image + src={penguin} + alt="a penguin" + width={300} + height={400} + fit="scale-down" + /> +</div> + +<div class="fit-outside"> + <Image + src={penguin} + alt="a penguin" + width={300} + height={400} + fit="outside" + /> +</div> + +<div class="fit-inside"> + <Image + src={penguin} + alt="a penguin" + width={300} + height={400} + fit="inside" + /> +</div> + +<div class="too-large"> + <Image + src={penguin} + alt="a penguin" + width={3000} + height={2000} + fit="cover" + /> +</div> diff --git a/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro b/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro index 654078992556..442f4ffb0e17 100644 --- a/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro +++ b/packages/astro/test/fixtures/core-image-layout/src/pages/fit.astro @@ -31,5 +31,5 @@ import penguin from "../assets/penguin.jpg"; <Image src={penguin} alt="a penguin" fit="unsupported" /> </div> <div id="position"> - <Image src={penguin} alt="a penguin" position="top left" /> + <Image src={penguin} alt="a penguin" position="right top" /> </div> From 41c2c7bd639b8664595efda50b7462a4fc4e2f7d Mon Sep 17 00:00:00 2001 From: Matt Kane <m@mk.gg> Date: Tue, 12 Nov 2024 16:05:11 +0000 Subject: [PATCH 7/7] Changes from review --- packages/astro/src/assets/consts.ts | 10 +++++++++- packages/astro/src/assets/internal.ts | 12 ++++++------ packages/astro/src/assets/services/sharp.ts | 4 ++-- packages/astro/src/types/public/config.ts | 2 +- packages/astro/test/core-image-layout.test.js | 4 +--- packages/astro/test/core-image-service.test.js | 8 ++------ 6 files changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/astro/src/assets/consts.ts b/packages/astro/src/assets/consts.ts index cc9e80760702..5fae641ae462 100644 --- a/packages/astro/src/assets/consts.ts +++ b/packages/astro/src/assets/consts.ts @@ -26,4 +26,12 @@ export const VALID_SUPPORTED_FORMATS = [ ] as const; export const DEFAULT_OUTPUT_FORMAT = 'webp' as const; export const VALID_OUTPUT_FORMATS = ['avif', 'png', 'webp', 'jpeg', 'jpg', 'svg'] as const; -export const DEFAULT_HASH_PROPS = ['src', 'width', 'height', 'format', 'quality', 'fit', 'position']; +export const DEFAULT_HASH_PROPS = [ + 'src', + 'width', + 'height', + 'format', + 'quality', + 'fit', + 'position', +]; diff --git a/packages/astro/src/assets/internal.ts b/packages/astro/src/assets/internal.ts index 235f1115be46..fb1733dcf73b 100644 --- a/packages/astro/src/assets/internal.ts +++ b/packages/astro/src/assets/internal.ts @@ -2,6 +2,12 @@ import { isRemotePath } from '@astrojs/internal-helpers/path'; import { AstroError, AstroErrorData } from '../core/errors/index.js'; import type { AstroConfig } from '../types/public/config.js'; import { DEFAULT_HASH_PROPS } from './consts.js'; +import { + DEFAULT_RESOLUTIONS, + LIMITED_RESOLUTIONS, + getSizesAttribute, + getWidths, +} from './layout.js'; import { type ImageService, isLocalService } from './services/service.js'; import { type GetImageResult, @@ -12,12 +18,6 @@ import { } from './types.js'; import { isESMImportedImage, isRemoteImage, resolveSrc } from './utils/imageKind.js'; import { inferRemoteSize } from './utils/remoteProbe.js'; -import { - DEFAULT_RESOLUTIONS, - getSizesAttribute, - getWidths, - LIMITED_RESOLUTIONS, -} from './layout.js'; export async function getConfiguredImageService(): Promise<ImageService> { if (!globalThis?.astroAsset?.imageService) { diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index 81e5822527b4..bbae39eb093a 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -44,8 +44,8 @@ const fitMap: Record<ImageFit, keyof FitEnum> = { cover: 'cover', none: 'outside', 'scale-down': 'inside', - 'outside': 'outside', - 'inside': 'inside', + outside: 'outside', + inside: 'inside', }; const sharpService: LocalImageService<SharpImageServiceConfig> = { diff --git a/packages/astro/src/types/public/config.ts b/packages/astro/src/types/public/config.ts index 27d5a286a7cf..fd1617d89700 100644 --- a/packages/astro/src/types/public/config.ts +++ b/packages/astro/src/types/public/config.ts @@ -6,6 +6,7 @@ import type { ShikiConfig, } from '@astrojs/markdown-remark'; import type { UserConfig as OriginalViteUserConfig, SSROptions as ViteSSROptions } from 'vite'; +import type { ImageFit, ImageLayout } from '../../assets/types.js'; import type { RemotePattern } from '../../assets/utils/remotePattern.js'; import type { AssetsPrefix } from '../../core/app/types.js'; import type { AstroConfigType } from '../../core/config/schema.js'; @@ -13,7 +14,6 @@ import type { REDIRECT_STATUS_CODES } from '../../core/constants.js'; import type { Logger, LoggerLevel } from '../../core/logger/core.js'; import type { EnvSchema } from '../../env/schema.js'; import type { AstroIntegration } from './integrations.js'; -import type { ImageFit, ImageLayout } from '../../assets/types.js'; export type Locales = (string | { codes: string[]; path: string })[]; diff --git a/packages/astro/test/core-image-layout.test.js b/packages/astro/test/core-image-layout.test.js index eb42656da082..e2f1c954e8f8 100644 --- a/packages/astro/test/core-image-layout.test.js +++ b/packages/astro/test/core-image-layout.test.js @@ -5,8 +5,8 @@ import * as cheerio from 'cheerio'; import parseSrcset from 'parse-srcset'; import { Logger } from '../dist/core/logger/core.js'; import { testImageService } from './test-image-service.js'; -import { loadFixture } from './test-utils.js'; import { testRemoteImageService } from './test-remote-image-service.js'; +import { loadFixture } from './test-utils.js'; describe('astro:image:layout', () => { /** @type {import('./test-utils').Fixture} */ @@ -15,8 +15,6 @@ describe('astro:image:layout', () => { describe('local image service', () => { /** @type {import('./test-utils').DevServer} */ let devServer; - /** @type {Array<{ type: any, level: 'error', message: string; }>} */ - let logs = []; before(async () => { fixture = await loadFixture({ diff --git a/packages/astro/test/core-image-service.test.js b/packages/astro/test/core-image-service.test.js index 6e21196f69e6..0c75ed484f69 100644 --- a/packages/astro/test/core-image-service.test.js +++ b/packages/astro/test/core-image-service.test.js @@ -1,9 +1,9 @@ import assert from 'node:assert/strict'; import { after, before, describe, it } from 'node:test'; +import { removeDir } from '@astrojs/internal-helpers/fs'; import * as cheerio from 'cheerio'; -import { loadFixture } from './test-utils.js'; import { lookup as probe } from '../dist/assets/utils/vendor/image-size/lookup.js'; -import { removeDir } from '@astrojs/internal-helpers/fs'; +import { loadFixture } from './test-utils.js'; async function getImageDimensionsFromFixture(fixture, path) { /** @type { Response } */ @@ -26,8 +26,6 @@ describe('astro image service', () => { describe('dev image service', () => { /** @type {import('./test-utils').DevServer} */ let devServer; - /** @type {Array<{ type: any, level: 'error', message: string; }>} */ - let logs = []; before(async () => { fixture = await loadFixture({ @@ -149,7 +147,6 @@ describe('astro image service', () => { describe('generated images', () => { let $; - let src; before(async () => { const html = await fixture.readFile('/build/index.html'); $ = cheerio.load(html); @@ -204,7 +201,6 @@ describe('astro image service', () => { assert.equal(width, originalWidth); assert.equal(height, originalHeight); }); - }); }); });