From f5fddafc248bb1ef57b7349bfecc25539ae2b5ea Mon Sep 17 00:00:00 2001 From: Erika <3019731+Princesseuh@users.noreply.github.com> Date: Fri, 17 Mar 2023 13:29:25 +0100 Subject: [PATCH] Add validateOptions hook to Image Service API (#6555) * feat(assets): Add a validateOptions hooks to set default and do error handling * chore: changeset --- .changeset/tidy-dryers-add.md | 6 ++ packages/astro/src/assets/internal.ts | 13 ++- packages/astro/src/assets/services/service.ts | 82 ++++++++++++------- packages/astro/src/assets/services/sharp.ts | 9 +- packages/astro/src/assets/services/squoosh.ts | 6 +- .../astro/src/vite-plugin-markdown/index.ts | 16 +++- packages/astro/test/core-image.test.js | 7 ++ .../test/fixtures/core-image/service.mjs | 3 + packages/markdown/remark/src/rehype-images.ts | 8 +- 9 files changed, 99 insertions(+), 51 deletions(-) create mode 100644 .changeset/tidy-dryers-add.md diff --git a/.changeset/tidy-dryers-add.md b/.changeset/tidy-dryers-add.md new file mode 100644 index 000000000..f47ffdbda --- /dev/null +++ b/.changeset/tidy-dryers-add.md @@ -0,0 +1,6 @@ +--- +'astro': patch +'@astrojs/markdown-remark': patch +--- + +Add a `validateOptions` hook to the Image Service API in order to set default options and validate the passed options diff --git a/packages/astro/src/assets/internal.ts b/packages/astro/src/assets/internal.ts index 343c9e8b5..e4249f212 100644 --- a/packages/astro/src/assets/internal.ts +++ b/packages/astro/src/assets/internal.ts @@ -27,6 +27,7 @@ export async function getConfiguredImageService(): Promise { } interface GetImageResult { + rawOptions: ImageTransform; options: ImageTransform; src: string; attributes: Record; @@ -50,17 +51,21 @@ interface GetImageResult { */ export async function getImage(options: ImageTransform): Promise { const service = await getConfiguredImageService(); - let imageURL = service.getURL(options); + const validatedOptions = service.validateOptions ? service.validateOptions(options) : options; + + let imageURL = service.getURL(validatedOptions); // In build and for local services, we need to collect the requested parameters so we can generate the final images if (isLocalService(service) && globalThis.astroAsset.addStaticImage) { - imageURL = globalThis.astroAsset.addStaticImage(options); + imageURL = globalThis.astroAsset.addStaticImage(validatedOptions); } return { - options, + rawOptions: options, + options: validatedOptions, src: imageURL, - attributes: service.getHTMLAttributes !== undefined ? service.getHTMLAttributes(options) : {}, + attributes: + service.getHTMLAttributes !== undefined ? service.getHTMLAttributes(validatedOptions) : {}, }; } diff --git a/packages/astro/src/assets/services/service.ts b/packages/astro/src/assets/services/service.ts index bb320da05..5b1a1b80e 100644 --- a/packages/astro/src/assets/services/service.ts +++ b/packages/astro/src/assets/services/service.ts @@ -39,6 +39,15 @@ interface SharedServiceProps { * In most cases, you'll want to return directly what your user supplied you, minus the attributes that were used to generate the image. */ getHTMLAttributes?: (options: ImageTransform) => Record; + /** + * Validate and return the options passed by the user. + * + * This method is useful to present errors to users who have entered invalid options. + * For instance, if they are missing a required property or have entered an invalid image format. + * + * This method should returns options, and can be used to set defaults (ex: a default output format to be used if the user didn't specify one.) + */ + validateOptions?: (options: ImageTransform) => ImageTransform; } export type ExternalImageService = SharedServiceProps; @@ -69,7 +78,7 @@ export type BaseServiceTransform = { src: string; width?: number; height?: number; - format?: string | null; + format: string; quality?: string | null; }; @@ -94,6 +103,45 @@ export type BaseServiceTransform = { * */ export const baseService: Omit = { + validateOptions(options) { + if (!isESMImportedImage(options.src)) { + // For remote images, width and height are explicitly required as we can't infer them from the file + let missingDimension: 'width' | 'height' | 'both' | undefined; + if (!options.width && !options.height) { + missingDimension = 'both'; + } else if (!options.width && options.height) { + missingDimension = 'width'; + } else if (options.width && !options.height) { + missingDimension = 'height'; + } + + if (missingDimension) { + throw new AstroError({ + ...AstroErrorData.MissingImageDimension, + message: AstroErrorData.MissingImageDimension.message(missingDimension, options.src), + }); + } + } else { + if (!VALID_INPUT_FORMATS.includes(options.src.format as any)) { + throw new AstroError({ + ...AstroErrorData.UnsupportedImageFormat, + message: AstroErrorData.UnsupportedImageFormat.message( + options.src.format, + options.src.src, + VALID_INPUT_FORMATS + ), + }); + } + } + + // If the user didn't specify a format, we'll default to `webp`. It offers the best ratio of compatibility / quality + // In the future, hopefully we can replace this with `avif`, alas, Edge. See https://caniuse.com/avif + if (!options.format) { + options.format = 'webp'; + } + + return options; + }, getHTMLAttributes(options) { let targetWidth = options.width; let targetHeight = options.height; @@ -123,39 +171,11 @@ export const baseService: Omit = { }; }, getURL(options: ImageTransform) { + // Both our currently available local services don't handle remote images, so we return the path as is. if (!isESMImportedImage(options.src)) { - // For remote images, width and height are explicitly required as we can't infer them from the file - let missingDimension: 'width' | 'height' | 'both' | undefined; - if (!options.width && !options.height) { - missingDimension = 'both'; - } else if (!options.width && options.height) { - missingDimension = 'width'; - } else if (options.width && !options.height) { - missingDimension = 'height'; - } - - if (missingDimension) { - throw new AstroError({ - ...AstroErrorData.MissingImageDimension, - message: AstroErrorData.MissingImageDimension.message(missingDimension, options.src), - }); - } - - // Both our currently available local services don't handle remote images, so we return the path as is. return options.src; } - if (!VALID_INPUT_FORMATS.includes(options.src.format as any)) { - throw new AstroError({ - ...AstroErrorData.UnsupportedImageFormat, - message: AstroErrorData.UnsupportedImageFormat.message( - options.src.format, - options.src.src, - VALID_INPUT_FORMATS - ), - }); - } - const searchParams = new URLSearchParams(); searchParams.append('href', options.src.src); @@ -177,7 +197,7 @@ export const baseService: Omit = { src: params.get('href')!, width: params.has('w') ? parseInt(params.get('w')!) : undefined, height: params.has('h') ? parseInt(params.get('h')!) : undefined, - format: params.get('f') as OutputFormat | null, + format: params.get('f') as OutputFormat, quality: params.get('q'), }; diff --git a/packages/astro/src/assets/services/sharp.ts b/packages/astro/src/assets/services/sharp.ts index a79c2c33e..4923bffe3 100644 --- a/packages/astro/src/assets/services/sharp.ts +++ b/packages/astro/src/assets/services/sharp.ts @@ -23,19 +23,14 @@ async function loadSharp() { } const sharpService: LocalImageService = { + validateOptions: baseService.validateOptions, getURL: baseService.getURL, parseURL: baseService.parseURL, getHTMLAttributes: baseService.getHTMLAttributes, async transform(inputBuffer, transformOptions) { if (!sharp) sharp = await loadSharp(); - const transform: BaseServiceTransform = transformOptions; - - // If the user didn't specify a format, we'll default to `webp`. It offers the best ratio of compatibility / quality - // In the future, hopefully we can replace this with `avif`, alas, Edge. See https://caniuse.com/avif - if (!transform.format) { - transform.format = 'webp'; - } + const transform: BaseServiceTransform = transformOptions as BaseServiceTransform; let result = sharp(inputBuffer, { failOnError: false, pages: -1 }); diff --git a/packages/astro/src/assets/services/squoosh.ts b/packages/astro/src/assets/services/squoosh.ts index b726b4034..3aeef90f4 100644 --- a/packages/astro/src/assets/services/squoosh.ts +++ b/packages/astro/src/assets/services/squoosh.ts @@ -21,16 +21,14 @@ const qualityTable: Record, Record(); + let imageService: ImageService | undefined = undefined; + async function resolveImage(this: PluginContext, fileId: string, path: string) { const resolved = await this.resolve(path, fileId); if (!resolved) return path; @@ -93,7 +96,6 @@ export default function markdown({ settings, logging }: AstroPluginOptions): Plu const rawFile = await fs.promises.readFile(fileId, 'utf-8'); const raw = safeMatter(rawFile, id); - let imageService = undefined; if (settings.config.experimental.assets) { imageService = (await import(settings.config.image.service)).default; } @@ -221,10 +223,18 @@ export default function markdown({ settings, logging }: AstroPluginOptions): Plu } const fileName = this.getFileName(hash); image.src = npath.join(settings.config.base, fileName); - const optimized = globalThis.astroAsset.addStaticImage!({ src: image }); + + // TODO: This part recreates code we already have for content collection and normal ESM imports. + // It might be possible to refactor so it also uses `emitESMImage`? - erika, 2023-03-15 + const options = { src: image }; + const validatedOptions = imageService?.validateOptions + ? imageService.validateOptions(options) + : options; + + const optimized = globalThis.astroAsset.addStaticImage!(validatedOptions); optimizedPaths.set(hash, optimized); } - output.code = output.code.replace(/ASTRO_ASSET_MD_([0-9a-z]{8})/, (_str, hash) => { + output.code = output.code.replaceAll(/ASTRO_ASSET_MD_([0-9a-z]{8})/gm, (_str, hash) => { const optimizedName = optimizedPaths.get(hash); return optimizedName || this.getFileName(hash); }); diff --git a/packages/astro/test/core-image.test.js b/packages/astro/test/core-image.test.js index 59003024f..eb84e1250 100644 --- a/packages/astro/test/core-image.test.js +++ b/packages/astro/test/core-image.test.js @@ -308,6 +308,13 @@ describe('astro:image', () => { expect(data).to.be.an.instanceOf(Buffer); }); + it('writes out images to dist folder with proper extension if no format was passed', async () => { + const html = await fixture.readFile('/index.html'); + const $ = cheerio.load(html); + const src = $('#local img').attr('src'); + expect(src.endsWith('.webp')).to.be.true; + }); + it('getImage() usage also written', async () => { const html = await fixture.readFile('/get-image/index.html'); const $ = cheerio.load(html); diff --git a/packages/astro/test/fixtures/core-image/service.mjs b/packages/astro/test/fixtures/core-image/service.mjs index 7db1cdcc5..dfede13b3 100644 --- a/packages/astro/test/fixtures/core-image/service.mjs +++ b/packages/astro/test/fixtures/core-image/service.mjs @@ -1,6 +1,9 @@ import squoosh from 'astro/assets/services/squoosh'; const service = { + validateOptions(options) { + return squoosh.validateOptions(options); + }, getURL(options) { return squoosh.getURL(options); }, diff --git a/packages/markdown/remark/src/rehype-images.ts b/packages/markdown/remark/src/rehype-images.ts index fcd39cd60..1d615c8b5 100644 --- a/packages/markdown/remark/src/rehype-images.ts +++ b/packages/markdown/remark/src/rehype-images.ts @@ -38,11 +38,15 @@ export function rehypeImages(imageService: any, assetsDir: URL | undefined, getI alt: node.properties.alt, }; - const imageURL = imageService.getURL(options); + const validatedOptions = imageService.validateOptions + ? imageService.validateOptions(options) + : options; + + const imageURL = imageService.getURL(validatedOptions); node.properties = Object.assign(node.properties, { src: imageURL, ...(imageService.getHTMLAttributes !== undefined - ? imageService.getHTMLAttributes(options) + ? imageService.getHTMLAttributes(validatedOptions) : {}), }); }