Skip to content

Commit

Permalink
fix(assets): Propagate sizes attribute on all sources (#8986)
Browse files Browse the repository at this point in the history
* fix(assets): Propagate `sizes` attribute on all sources

* refactor: small refactor exposed srcSet types

* test: update test with a sizes

* chore: changeset

* fix: use a type import
  • Loading branch information
Princesseuh authored Nov 2, 2023
1 parent 3cb1098 commit 910eb00
Show file tree
Hide file tree
Showing 8 changed files with 44 additions and 20 deletions.
5 changes: 5 additions & 0 deletions .changeset/tasty-oranges-bathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix `sizes` attribute not being present on `source` elements when using it on the Picture component
4 changes: 2 additions & 2 deletions packages/astro/components/Image.astro
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
---
import { getImage, type LocalImageProps, type RemoteImageProps } from 'astro:assets';
import { AstroError, AstroErrorData } from '../dist/core/errors/index.js';
import type { HTMLAttributes } from '../types';
// The TypeScript diagnostic for JSX props uses the last member of the union to suggest props, so it would be better for
// LocalImageProps to be last. Unfortunately, when we do this the error messages that remote images get are complete nonsense
Expand All @@ -24,8 +25,7 @@ if (typeof props.height === 'string') {
const image = await getImage(props);
const additionalAttributes: Record<string, any> = {};
const additionalAttributes: HTMLAttributes<'img'> = {};
if (image.srcSet.values.length > 0) {
additionalAttributes.srcset = image.srcSet.attribute;
}
Expand Down
21 changes: 17 additions & 4 deletions packages/astro/components/Picture.astro
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,16 @@ const fallbackImage = await getImage({
densities: props.densities,
});
const additionalAttributes: Record<string, any> = {};
const imgAdditionalAttributes: HTMLAttributes<'img'> = {};
const sourceAdditionaAttributes: HTMLAttributes<'source'> = {};
// Propagate the `sizes` attribute to the `source` elements
if (props.sizes) {
sourceAdditionaAttributes.sizes = props.sizes;
}
if (fallbackImage.srcSet.values.length > 0) {
additionalAttributes.srcset = fallbackImage.srcSet.attribute;
imgAdditionalAttributes.srcset = fallbackImage.srcSet.attribute;
}
---

Expand All @@ -62,8 +69,14 @@ if (fallbackImage.srcSet.values.length > 0) {
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} />;
return (
<source
srcset={srcsetAttribute}
type={'image/' + image.options.format}
{...sourceAdditionaAttributes}
/>
);
})
}
<img src={fallbackImage.src} {...additionalAttributes} {...fallbackImage.attributes} />
<img src={fallbackImage.src} {...imgAdditionalAttributes} {...fallbackImage.attributes} />
</picture>
2 changes: 2 additions & 0 deletions packages/astro/src/assets/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export async function getImage(
let imageURL = await service.getURL(validatedOptions, imageConfig);
let srcSets: SrcSetValue[] = await Promise.all(
srcSetTransforms.map(async (srcSet) => ({
transform: srcSet.transform,
url: await service.getURL(srcSet.transform, imageConfig),
descriptor: srcSet.descriptor,
attributes: srcSet.attributes,
Expand All @@ -115,6 +116,7 @@ export async function getImage(
) {
imageURL = globalThis.astroAsset.addStaticImage(validatedOptions);
srcSets = srcSetTransforms.map((srcSet) => ({
transform: srcSet.transform,
url: globalThis.astroAsset.addStaticImage!(srcSet.transform),
descriptor: srcSet.descriptor,
attributes: srcSet.attributes,
Expand Down
12 changes: 3 additions & 9 deletions packages/astro/src/assets/services/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { AstroError, AstroErrorData } from '../../core/errors/index.js';
import { isRemotePath, joinPaths } from '../../core/path.js';
import { DEFAULT_OUTPUT_FORMAT, VALID_SUPPORTED_FORMATS } from '../consts.js';
import { isESMImportedImage, isRemoteAllowed } from '../internal.js';
import type { ImageOutputFormat, ImageTransform } from '../types.js';
import type { ImageOutputFormat, ImageTransform, UnresolvedSrcSetValue } from '../types.js';

export type ImageService = LocalImageService | ExternalImageService;

Expand All @@ -28,12 +28,6 @@ type ImageConfig<T> = Omit<AstroConfig['image'], 'service'> & {
service: { entrypoint: string; config: T };
};

type SrcSetValue = {
transform: ImageTransform;
descriptor?: string;
attributes?: Record<string, any>;
};

interface SharedServiceProps<T extends Record<string, any> = Record<string, any>> {
/**
* Return the URL to the endpoint or URL your images are generated from.
Expand All @@ -53,7 +47,7 @@ interface SharedServiceProps<T extends Record<string, any> = Record<string, any>
getSrcSet?: (
options: ImageTransform,
imageConfig: ImageConfig<T>
) => SrcSetValue[] | Promise<SrcSetValue[]>;
) => UnresolvedSrcSetValue[] | Promise<UnresolvedSrcSetValue[]>;
/**
* Return any additional HTML attributes separate from `src` that your service requires to show the image properly.
*
Expand Down Expand Up @@ -233,7 +227,7 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
};
},
getSrcSet(options) {
const srcSet: SrcSetValue[] = [];
const srcSet: UnresolvedSrcSetValue[] = [];
const { targetWidth } = getTargetDimensions(options);
const { widths, densities } = options;
const targetFormat = options.format ?? DEFAULT_OUTPUT_FORMAT;
Expand Down
15 changes: 11 additions & 4 deletions packages/astro/src/assets/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,18 @@ export interface ImageMetadata {
orientation?: number;
}

export interface SrcSetValue {
url: string;
/**
* A yet to be completed with an url `SrcSetValue`. Other hooks will only see a resolved value, where the URL of the image has been added.
*/
export type UnresolvedSrcSetValue = {
transform: ImageTransform;
descriptor?: string;
attributes?: Record<string, string>;
}
attributes?: Record<string, any>;
};

export type SrcSetValue = UnresolvedSrcSetValue & {
url: string;
};

/**
* A yet to be resolved image transform. Used by `getImage`
Expand Down
3 changes: 3 additions & 0 deletions packages/astro/test/core-image.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ describe('astro:image', () => {
expect($img).to.have.a.lengthOf(1);
expect($picture).to.have.a.lengthOf(1);
expect($source).to.have.a.lengthOf(1);
expect($source.attr('sizes')).to.equal(
'(max-width: 448px) 400px, (max-width: 810px) 750px, 1050px'
);

const srcset2 = parseSrcset($source.attr('srcset'));
expect(srcset2.every((src) => src.url.startsWith('/_image'))).to.equal(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import myImage from "../assets/penguin1.jpg";
</div>

<div id="picture-widths">
<Picture src={myImage} width={Math.round(myImage.width / 2)} alt="A penguin" widths={[myImage.width]} />
<Picture src={myImage} width={Math.round(myImage.width / 2)} alt="A penguin" widths={[myImage.width]} sizes="(max-width: 448px) 400px, (max-width: 810px) 750px, 1050px" />
</div>

<div id="picture-fallback">
Expand Down

0 comments on commit 910eb00

Please sign in to comment.