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

fix(assets): Propagate sizes attribute on all sources #8986

Merged
merged 6 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
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
Loading