fix: address the code review findings

packages/bundler-utils/src/generate.spec.ts

@@ -1,7 +1,8 @@
 import {
   describe,
   it,
-  expect
+  expect,
+  vi
 } from 'vitest'
 import sharp from 'sharp'
 import { generateSrcSetModule } from './generate.ts'
@@ -118,6 +119,26 @@ describe('bundler-utils', () => {
         expect(module).toContain('"jpg160"')
       })
 
+      it('should deduplicate variants of overlapping fallthrough rules', async () => {
+        const image = await createImage()
+        const emitImage = vi.fn(emitToPath)
+        const module = await generateSrcSetModule(image, {}, {
+          rules: [
+            {
+              fallthrough: true,
+              width: [0.5, 1]
+            },
+            {
+              width: [320, 0.25]
+            }
+          ]
+        }, emitImage)
+
+        expect(emitImage).toHaveBeenCalledTimes(3)
+        expect(module.match(/"jpg320":/g)?.length).toBe(1)
+        expect(module).toContain('"jpg160"')
+      })
+
       it('should inline placeholder data-url', async () => {
         const image = await createImage()
         const module = await generateSrcSetModule(
@@ -133,6 +154,119 @@ describe('bundler-utils', () => {
         expect(module).toMatch(/export const placeholder = "data:image\/webp;base64,[^"]+";/)
       })
 
+      it('should keep the configured placeholder options for the query flag', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {
+          placeholder: true
+        }, {
+          skipOptimization: true,
+          placeholder: {
+            format: 'jpg'
+          }
+        }, emitToPath)
+
+        expect(module).toMatch(/export const placeholder = "data:image\/jpeg;base64,/)
+      })
+
+      it('should disable the placeholder from the query', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {
+          placeholder: false
+        }, {
+          skipOptimization: true,
+          placeholder: {
+            format: 'jpg'
+          }
+        }, emitToPath)
+
+        expect(module).toContain('export const placeholder = undefined;')
+      })
+
+      it('should select the requested format at the first width of the rule', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {
+          select: {
+            format: 'webp'
+          }
+        }, {
+          rules: [
+            {
+              format: ['jpg', 'webp'],
+              width: [1, 0.5]
+            }
+          ]
+        }, emitToPath)
+
+        expect(module).toContain('const url = "/images/image.webp";')
+      })
+
+      it('should select the requested width in the first format of the rule', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {
+          select: {
+            width: 320
+          }
+        }, {
+          rules: [
+            {
+              format: ['webp', 'jpg'],
+              width: [1, 0.5]
+            }
+          ]
+        }, emitToPath)
+
+        expect(module).toContain('const url = "/images/image@320w.webp";')
+      })
+
+      it('should prefer the select from the query over the options', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {
+          select: {
+            id: 'webp320'
+          }
+        }, {
+          select: {
+            id: 'jpg320'
+          },
+          rules: [
+            {
+              format: ['jpg', 'webp'],
+              width: [0.5]
+            }
+          ]
+        }, emitToPath)
+
+        expect(module).toContain('const url = "/images/image@320w.webp";')
+      })
+
+      it('should select the original variant without an explicit select', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {}, {
+          rules: [
+            {
+              format: ['webp', 'jpg'],
+              width: [1, 0.5]
+            }
+          ]
+        }, emitToPath)
+
+        expect(module).toContain('const url = "/images/image.jpg";')
+      })
+
+      it('should fall back to the first variant when nothing matches the select', async () => {
+        const image = await createImage()
+        const module = await generateSrcSetModule(image, {}, {
+          rules: [
+            {
+              format: ['webp', 'jpg'],
+              width: [0.5]
+            }
+          ]
+        }, emitToPath)
+
+        expect(module).toContain('const url = "/images/image@320w.webp";')
+      })
+
       it('should build public path expression without a plain public path', async () => {
         const image = await createImage()
         const module = await generateSrcSetModule(image, {}, {

packages/bundler-utils/src/generate.ts

@@ -3,7 +3,6 @@ import {
   type ImageSource,
   SrcSetGenerator,
   getImageMetadata,
-  matchImage,
   mimeTypes
 } from '@srcset/core'
 import type { QueryOptions } from './query.ts'
@@ -45,41 +44,46 @@ export async function generateSrcSetModule(
     limit
   })
   const metadata = await getImageMetadata(source)
+  // The query flag only switches the placeholder on and off: the configured
+  // options stay, so `?placeholder` does not fall back to the defaults.
+  const placeholderOptions = query.placeholder === undefined
+    ? options.placeholder
+    : query.placeholder && (options.placeholder ?? true)
   const placeholder = await createPlaceholder(
     source,
     metadata,
-    query.placeholder ?? options.placeholder,
+    placeholderOptions,
     limit,
     options.cache
   )
-  const select = {
-    format: metadata.format,
-    width: metadata.width,
+  const userSelect = {
     ...options.select,
     ...query.select
   }
-  const srcSet: SrcSetModuleEntry[] = []
-
-  for (const rule of rules) {
-    if (!await matchImage(source, rule.match)) {
-      continue
-    }
-
-    for await (const image of generator.generate(source, rule)) {
-      srcSet.push({
-        id: resourceId(image.width, image.originMultiplier ?? image.width, image.format),
-        format: image.format,
-        type: mimeTypes[image.format],
-        width: image.width,
-        height: image.height,
-        originMultiplier: image.originMultiplier,
-        url: emitImage(image)
-      })
+  const hasUserSelect = userSelect.id !== undefined
+    || userSelect.format !== undefined
+    || userSelect.width !== undefined
+  // The implicit selection describes the original image and applies only when
+  // nothing is selected explicitly: mixed into a partial selection it would
+  // defeat the half the user did not specify.
+  const select = hasUserSelect
+    ? userSelect
+    : {
+      format: metadata.format,
+      width: metadata.width
     }
+  const srcSet: SrcSetModuleEntry[] = []
 
-    if (!rule.fallthrough) {
-      break
-    }
+  for await (const image of generator.generateAll(source, rules)) {
+    srcSet.push({
+      id: resourceId(image.width, image.originMultiplier ?? image.width, image.format),
+      format: image.format,
+      type: mimeTypes[image.format],
+      width: image.width,
+      height: image.height,
+      originMultiplier: image.originMultiplier,
+      url: emitImage(image)
+    })
   }
 
   return createModuleString(select, srcSet, placeholder)

packages/bundler-utils/src/generate.types.ts

@@ -1,11 +1,9 @@
 import type {
   SrcSetImage,
-  SrcSetGeneratorOptions
+  SrcSetGeneratorOptions,
+  SrcSetRule
 } from '@srcset/core'
-import type {
-  SrcSetRule,
-  SrcSetImagePaths
-} from './types.ts'
+import type { SrcSetImagePaths } from './types.ts'
 import type {
   SrcSetEntrySelect,
   ResourceIdFormatter

packages/bundler-utils/src/placeholder.ts

@@ -77,7 +77,9 @@ export async function createPlaceholder(
 
   const createImage = async (): Promise<SrcSetImage> => {
     const contents = await limit(() => {
-      const pipeline = sharp(source.contents).resize({
+      const pipeline = sharp(source.contents, {
+        autoOrient: true
+      }).resize({
         width,
         withoutEnlargement: true
       })

packages/bundler-utils/src/query.ts

@@ -1,4 +1,4 @@
-import type { SrcSetRule } from './types.ts'
+import type { SrcSetRule } from '@srcset/core'
 import type { SrcSetEntrySelect } from './module.ts'
 
 export interface QueryOptions {

packages/bundler-utils/src/types.ts

@@ -1,8 +1,3 @@
-import type {
-  Matcher,
-  GenerateOptions
-} from '@srcset/core'
-
 /**
  * Paths of an image emitted on the bundler side.
  */
@@ -22,18 +17,3 @@ export interface SrcSetImagePaths {
    */
   publicPathExpression?: string
 }
-
-/**
- * Rule to generate image variants: match options plus generate options.
- */
-export interface SrcSetRule extends GenerateOptions {
-  /**
-   * Rule(s) to match the image: glob, media query or matcher function.
-   */
-  match?: Matcher | Matcher[]
-  /**
-   * Keep matching the rest of the rules after this rule matched.
-   * By default the first matched rule is the only one applied.
-   */
-  fallthrough?: boolean
-}

packages/cli/src/args.spec.ts

@@ -9,6 +9,12 @@ import { parseCliArgs } from './args.ts'
 describe('cli', () => {
   describe('args', () => {
     describe('parseCliArgs', () => {
+      it('should reject an unrecognised option', () => {
+        setArgs('images/**/*.jpg', '--formats', 'webp', '--dest', 'dist')
+
+        expect(() => parseCliArgs()).toThrow('Unknown option: "--formats"')
+      })
+
       it('should parse sources and options', () => {
         setArgs(
           'images/**/*.jpg',

packages/cli/src/args.ts

@@ -1,4 +1,7 @@
-import type { ImageFormat } from '@srcset/core'
+import type {
+  ImageFormat,
+  SrcSetRule
+} from '@srcset/core'
 import {
   rest,
   alias,
@@ -7,7 +10,6 @@ import {
   option,
   readOptions
 } from 'argue-cli'
-import type { SrcSetCliRule } from './types.ts'
 
 export const usage = `srcset [...sources] [...options]
 
@@ -28,7 +30,7 @@ export interface CliArgs {
   help: boolean
   verbose: boolean | undefined
   sources: string[]
-  rule: SrcSetCliRule | null
+  rule: SrcSetRule | null
   skipOptimization: boolean | undefined
   scalingUp: boolean | undefined
   dest: string | undefined
@@ -64,7 +66,7 @@ export function parseCliArgs(): CliArgs {
     option(alias('config', 'c'), String),
     option('concurrency', Number)
   )
-  const rule: SrcSetCliRule = {
+  const rule: SrcSetRule = {
     ...match && {
       match
     },
@@ -75,11 +77,19 @@ export function parseCliArgs(): CliArgs {
       format: format as ImageFormat[]
     }
   }
+  const sources = rest()
+  // Whatever the readers did not take stays in `argv`: a leftover flag is a
+  // typo, and treating it as a glob would quietly match nothing.
+  const unknownOption = sources.find(source => source.startsWith('-'))
+
+  if (unknownOption) {
+    throw new Error(`Unknown option: "${unknownOption}".`)
+  }
 
   return {
     help: Boolean(help),
     verbose,
-    sources: rest(),
+    sources,
     rule: Object.keys(rule).length ? rule : null,
     skipOptimization,
     scalingUp,

packages/cli/src/cli.ts

@@ -7,14 +7,14 @@ import {
 import { loadConfig } from './config.ts'
 import { run } from './run.ts'
 
-const args = parseCliArgs()
+try {
+  const args = parseCliArgs()
 
-if (args.help) {
-  console.info(usage)
-  process.exit(0)
-}
+  if (args.help) {
+    console.info(usage)
+    process.exit(0)
+  }
 
-try {
   const config = await loadConfig(args.config)
   const options: SrcSetCliOptions = {
     ...config,