fix: remote srcset images not being resized and deduplication not wor…

…king in certain cases (#8823) * fix: remote `srcset` images not being resized * fix: hash keys ordered to reduce duplicate assets * fix: move to workaround for hashing function * fix: rework transform logic for densities and widths * chore: changeset * test: add tests * fix: forced base srcset when using widths * fix: unnecessary coalescing * refactor: adjust with feedback --------- Co-authored-by: Matteo Manfredi <matteo@manfredi.io>
withastro · Oct 16, 2023 · 8946f2a · 8946f2a
1 parent b405b03
commit 8946f2a
Show file tree

Hide file tree

Showing 9 changed files with 213 additions and 65 deletions.
diff --git a/.changeset/brown-pumpkins-clean.md b/.changeset/brown-pumpkins-clean.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix duplicate images being created in some cases when using densities and/or widths
diff --git a/.changeset/serious-news-yell.md b/.changeset/serious-news-yell.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+fix remote srcset images not being resized
diff --git a/packages/astro/components/Picture.astro b/packages/astro/components/Picture.astro
@@ -57,14 +57,13 @@ if (fallbackImage.srcSet.values.length > 0) {
 
 <picture {...pictureAttributes}>
 	{
-		Object.entries(optimizedImages).map(([_, image]) => (
-			<source
-				srcset={`${image.src}${
-					image.srcSet.values.length > 0 ? ' , ' + image.srcSet.attribute : ''
-				}`}
-				type={'image/' + image.options.format}
-			/>
-		))
+		Object.entries(optimizedImages).map(([_, image]) => {
+			const srcsetAttribute =
+				props.densities || (!props.densities && !props.widths)
+					? `${image.src}${image.srcSet.values.length > 0 ? ', ' + image.srcSet.attribute : ''}`
+					: image.srcSet.attribute;
+			return <source srcset={srcsetAttribute} type={'image/' + image.options.format} />;
+		})
 	}
 	<img src={fallbackImage.src} {...additionalAttributes} {...fallbackImage.attributes} />
 </picture>
diff --git a/packages/astro/package.json b/packages/astro/package.json
@@ -139,6 +139,7 @@
     "common-ancestor-path": "^1.0.1",
     "cookie": "^0.5.0",
     "debug": "^4.3.4",
+    "deterministic-object-hash": "^1.3.1",
     "devalue": "^4.3.2",
     "diff": "^5.1.0",
     "es-module-lexer": "^1.3.0",

diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts
@@ -213,6 +213,10 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
 			options.format = DEFAULT_OUTPUT_FORMAT;
 		}
 
+		// 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);
+
 		return options;
 	},
 	getHTMLAttributes(options) {
@@ -230,16 +234,33 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
 	},
 	getSrcSet(options) {
 		const srcSet: SrcSetValue[] = [];
-		const { targetWidth, targetHeight } = getTargetDimensions(options);
+		const { targetWidth } = getTargetDimensions(options);
 		const { widths, densities } = options;
 		const targetFormat = options.format ?? DEFAULT_OUTPUT_FORMAT;
 
-		const aspectRatio = targetWidth / targetHeight;
-		const imageWidth = isESMImportedImage(options.src) ? options.src.width : options.width;
-		const maxWidth = imageWidth ?? Infinity;
+		// For remote images, we don't know the original image's dimensions, so we cannot know the maximum width
+		// It is ultimately the user's responsibility to make sure they don't request images larger than the original
+		let imageWidth = options.width;
+		let maxWidth = Infinity;
+
+		// However, if it's an imported image, we can use the original image's width as a maximum width
+		if (isESMImportedImage(options.src)) {
+			imageWidth = options.src.width;
+			maxWidth = imageWidth;
+		}
 
-		// REFACTOR: Could we merge these two blocks?
+		// Since `widths` and `densities` ultimately control the width and height of the image,
+		// we don't want the dimensions the user specified, we'll create those ourselves.
+		const {
+			width: transformWidth,
+			height: transformHeight,
+			...transformWithoutDimensions
+		} = options;
+
+		// Collect widths to generate from specified densities or widths
+		const allWidths: { maxTargetWidth: number; descriptor: `${number}x` | `${number}w` }[] = [];
 		if (densities) {
+			// Densities can either be specified as numbers, or descriptors (ex: '1x'), we'll convert them all to numbers
 			const densityValues = densities.map((density) => {
 				if (typeof density === 'number') {
 					return density;
@@ -248,64 +269,49 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
 				}
 			});
 
+			// Calculate the widths for each density, rounding to avoid floats.
 			const densityWidths = densityValues
 				.sort()
 				.map((density) => Math.round(targetWidth * density));
 
-			densityWidths.forEach((width, index) => {
-				const maxTargetWidth = Math.min(width, maxWidth);
-
-				// If the user passed dimensions, we don't want to add it to the srcset
-				const { width: transformWidth, height: transformHeight, ...rest } = options;
-
-				const srcSetValue = {
-					transform: {
-						...rest,
-					},
-					descriptor: `${densityValues[index]}x`,
-					attributes: {
-						type: `image/${targetFormat}`,
-					},
-				};
-
-				// Only set width and height if they are different from the original image, to avoid duplicated final images
-				if (maxTargetWidth !== imageWidth) {
-					srcSetValue.transform.width = maxTargetWidth;
-					srcSetValue.transform.height = Math.round(maxTargetWidth / aspectRatio);
-				}
-
-				if (targetFormat !== options.format) {
-					srcSetValue.transform.format = targetFormat;
-				}
-
-				srcSet.push(srcSetValue);
-			});
+			allWidths.push(
+				...densityWidths.map((width, index) => ({
+					maxTargetWidth: Math.min(width, maxWidth),
+					descriptor: `${densityValues[index]}x` as const,
+				}))
+			);
 		} else if (widths) {
-			widths.forEach((width) => {
-				const maxTargetWidth = Math.min(width, maxWidth);
-
-				const { width: transformWidth, height: transformHeight, ...rest } = options;
-
-				const srcSetValue = {
-					transform: {
-						...rest,
-					},
-					descriptor: `${width}w`,
-					attributes: {
-						type: `image/${targetFormat}`,
-					},
-				};
-
-				if (maxTargetWidth !== imageWidth) {
-					srcSetValue.transform.width = maxTargetWidth;
-					srcSetValue.transform.height = Math.round(maxTargetWidth / aspectRatio);
-				}
+			allWidths.push(
+				...widths.map((width) => ({
+					maxTargetWidth: Math.min(width, maxWidth),
+					descriptor: `${width}w` as const,
+				}))
+			);
+		}
 
-				if (targetFormat !== options.format) {
-					srcSetValue.transform.format = targetFormat;
+		// Caution: The logic below is a bit tricky, as we need to make sure we don't generate the same image multiple times
+		// When making changes, make sure to test with different combinations of local/remote images widths, densities, and dimensions etc.
+		for (const { maxTargetWidth, descriptor } of allWidths) {
+			const srcSetTransform: ImageTransform = { ...transformWithoutDimensions };
+
+			// Only set the width if it's different from the original image's width, to avoid generating the same image multiple times
+			if (maxTargetWidth !== imageWidth) {
+				srcSetTransform.width = maxTargetWidth;
+			} else {
+				// If the width is the same as the original image's width, and we have both dimensions, it probably means
+				// it's a remote image, so we'll use the user's specified dimensions to avoid recreating the original image unnecessarily
+				if (options.width && options.height) {
+					srcSetTransform.width = options.width;
+					srcSetTransform.height = options.height;
 				}
+			}
 
-				srcSet.push(srcSetValue);
+			srcSet.push({
+				transform: srcSetTransform,
+				descriptor,
+				attributes: {
+					type: `image/${targetFormat}`,
+				},
 			});
 		}
 
@@ -356,6 +362,17 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
 	},
 };
 
+/**
+ * Returns the final dimensions of an image based on the user's options.
+ *
+ * For local images:
+ * - If the user specified both width and height, we'll use those.
+ * - If the user specified only one of them, we'll use the original image's aspect ratio to calculate the other.
+ * - If the user didn't specify either, we'll use the original image's dimensions.
+ *
+ * For remote images:
+ * - Widths and heights are always required, so we'll use the user's specified width and height.
+ */
 function getTargetDimensions(options: ImageTransform) {
 	let targetWidth = options.width;
 	let targetHeight = options.height;

diff --git a/packages/astro/src/assets/utils/transformToPath.ts b/packages/astro/src/assets/utils/transformToPath.ts
@@ -1,3 +1,4 @@
+import { deterministicString } from 'deterministic-object-hash';
 import { basename, extname } from 'node:path';
 import { removeQueryString } from '../../core/path.js';
 import { shorthash } from '../../runtime/server/shorthash.js';
@@ -19,5 +20,5 @@ export function hashTransform(transform: ImageTransform, imageService: string) {
 	// Extract the fields we want to hash
 	const { alt, class: className, style, widths, densities, ...rest } = transform;
 	const hashFields = { ...rest, imageService };
-	return shorthash(JSON.stringify(hashFields));
+	return shorthash(deterministicString(hashFields));
 }
diff --git a/packages/astro/test/core-image.test.js b/packages/astro/test/core-image.test.js
@@ -225,7 +225,61 @@ describe('astro:image', () => {
 
 				const srcset2 = parseSrcset($source.attr('srcset'));
 				expect(srcset2.every((src) => src.url.startsWith('/_image'))).to.equal(true);
-				expect(srcset2.map((src) => src.w)).to.deep.equal([undefined, 207]);
+				expect(srcset2.map((src) => src.w)).to.deep.equal([207]);
+			});
+
+			it('properly deduplicate srcset images', async () => {
+				let res = await fixture.fetch('/srcset');
+				let html = await res.text();
+				$ = cheerio.load(html);
+
+				let localImage = $('#local-3-images img');
+				expect(
+					new Set([
+						...parseSrcset(localImage.attr('srcset')).map((src) => src.url),
+						localImage.attr('src'),
+					]).size
+				).to.equal(3);
+
+				let remoteImage = $('#remote-3-images img');
+				expect(
+					new Set([
+						...parseSrcset(remoteImage.attr('srcset')).map((src) => src.url),
+						remoteImage.attr('src'),
+					]).size
+				).to.equal(3);
+
+				let local1x = $('#local-1x img');
+				expect(
+					new Set([
+						...parseSrcset(local1x.attr('srcset')).map((src) => src.url),
+						local1x.attr('src'),
+					]).size
+				).to.equal(1);
+
+				let remote1x = $('#remote-1x img');
+				expect(
+					new Set([
+						...parseSrcset(remote1x.attr('srcset')).map((src) => src.url),
+						remote1x.attr('src'),
+					]).size
+				).to.equal(1);
+
+				let local2Widths = $('#local-2-widths img');
+				expect(
+					new Set([
+						...parseSrcset(local2Widths.attr('srcset')).map((src) => src.url),
+						local2Widths.attr('src'),
+					]).size
+				).to.equal(2);
+
+				let remote2Widths = $('#remote-2-widths img');
+				expect(
+					new Set([
+						...parseSrcset(remote2Widths.attr('srcset')).map((src) => src.url),
+						remote2Widths.attr('src'),
+					]).size
+				).to.equal(2);
 			});
 		});
 

diff --git a/packages/astro/test/fixtures/core-image/src/pages/srcset.astro b/packages/astro/test/fixtures/core-image/src/pages/srcset.astro
@@ -0,0 +1,59 @@
+---
+import { Image } from "astro:assets";
+import image from "../assets/penguin2.jpg";
+
+---
+
+<div id="local-3-images">
+	<!--
+		In this example, only three images should be generated :
+		- The base image
+		- The 1.5x image
+		- The 2x image
+	-->
+	<Image src={image} width={image.width / 2} densities={[1.5, 2]} alt="" />
+</div>
+
+<div id="remote-3-images">
+	<!--
+		In this example, only three images should be generated :
+		- The base image
+		- The 1.5x image
+		- The 2x image
+	-->
+	<Image src={"https://avatars.githubusercontent.com/u/622227?s=64"} width={32} height={32} densities={[1.5, 2]} alt="" />
+</div>
+
+<div id="local-1x">
+	<!--
+		In this example, only one image should be generated :
+		- The base image, 1x density should be the same as the base image
+	-->
+	<Image src={image} width={image.width / 2} densities={[1]} alt="" />
+</div>
+
+<div id="remote-1x">
+	<!--
+		In this example, only one image should be generated :
+		- The base image, 1x density should be the same as the base image
+	-->
+	<Image src={"https://avatars.githubusercontent.com/u/622227?s=64"} width={32} height={32} densities={[1]} alt="" />
+</div>
+
+<div id="local-2-widths">
+	<!--
+		In this example, only two images should be generated :
+		- The base image
+		- The 2x image
+	-->
+	<Image src={image} width={image.width / 2} widths={[image.width]} alt="" />
+</div>
+
+<div id="remote-2-widths">
+	<!--
+		In this example, only two images should be generated :
+		- The base image
+		- The 2x image
+	-->
+	<Image src={"https://avatars.githubusercontent.com/u/622227?s=64"} width={32} height={32} widths={[64]} alt="" />
+</div>
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml