feat(image): support images.loaderFile and emit srcSet from custom loaders#2700
feat(image): support images.loaderFile and emit srcSet from custom loaders#2700NathanDrake2406 wants to merge 5 commits into
Conversation
vinext replaces next/image with a from-scratch shim, so Next's build-time machinery for `images.loaderFile` — a bundler alias that swaps the default loader module — had no equivalent here. The option was accepted by config and then silently ignored, leaving remote images unoptimized. Generate a `virtual:vinext-image-loader` module from next.config so a configured loaderFile becomes the default loader for every image, local or remote. `loader: "custom"` without a loaderFile now reports the missing per-image `loader` prop instead of quietly falling back to /_next/image, and a missing or default-export-less loaderFile fails at config time. Custom loaders also never produced a srcSet: the loader ran once with `imgWidth ?? 0`, so `fill` images — which have no intrinsic width — sent `width: 0` into user loader code. Port Next's getWidths/generateImgAttrs so the loader is invoked once per candidate width, with the width-less case mapping to the deviceSizes ladder rather than 0. This also applies to getImageProps, which shared the same defect. Two existing tests asserted the old behavior of `src` being the declared width; upstream makes `src` the largest candidate so non-srcSet browsers get the highest-fidelity image. They are updated with the derivation. Fixes cloudflare#2699
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Strong PR — the diagnosis matches what I found independently while chasing the same issue on a real project (Next 16 + One concrete thing worth tightening before merge — the
So for a remote Minor, optional: Happy to see this land — it unblocks CDN/CMS images through |
Production getImageProps calls blocked remote sources before a custom loader could translate them, diverging from the Image component and breaking picture elements built from the returned props. Let an effective loader own remote URL generation in both paths, and preserve overrideSrc as the fallback source without discarding loader-generated candidates.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3707766599
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four review findings, all where loader selection and image rendering were
ordered wrongly relative to each other.
A named `images.loader` ("imgix", "cloudinary", "akamai") was only rejected
when paired with a `loaderFile`. On its own it fell past the early return and
nothing downstream caught it, so every image was quietly served from
/_next/image instead of the CDN the config named. Validate the loader value
before `loaderFile` is required.
`images.loader: "custom"` with no `loaderFile` generates a loader that reports
the missing per-image `loader` prop. The shim selected it only after the
`unoptimized` early return, so `<Image unoptimized>` and
`getImageProps({ unoptimized: true })` rendered the original URL from a config
that cannot produce URLs at all. Upstream throws above `generateImgAttrs`,
which is what handles `unoptimized`; mark the generated stub so the shim can
tell it from a working loader and honour that order. A valid loader is still
bypassed for unoptimized images.
Selecting a configured loader entered an <img> branch that ignored
`placeholder="blur"`, so an image that showed a placeholder under the built-in
loader silently lost it — and disagreed with `getImageProps`, which still
returned the blur style. Upstream merges `placeholderStyle` on every path.
Custom loaders received a forced `quality: 75` because the default was applied
at the loader seam rather than inside the built-in optimizer. A loader written
as `quality ?? 80`, or one letting a CDN choose, generated different URLs under
vinext than under Next.js. Pass the absence through; `imageOptimizationUrl`
still applies 75 at its own boundary.
Also drops the hand-authored changeset: CI generates changesets from
Conventional Commit subjects, so keeping it risks a duplicate release entry.
The shim's `images.loaderFile` / bare-`custom` branches were covered by a unit test that swapped `virtual:vinext-image-loader` with `vi.doMock`, which is exactly what `image/image-loader-unconfigured.ts` tells tests not to do. Move them into the existing plugin integration test, where a throwaway project with a real next.config is served by a real dev server, so the loader under test is the one the plugin generated. `getImageProps` runs from a client page: `next/image` is a "use client" module, so its exports are client references in the RSC environment and a route handler cannot call it, and the `loader` prop is a function that would otherwise have to cross the server/client boundary. The fixture loader now emits `quality=auto` when quality is absent. With `?? 75` it produced the same URL whether or not the shim forced a default, so the case could not fail.
2 participants
Overview
images.loaderFile/images.loaderwork, and make custom loaders emit a realsrcSetvirtual:vinext-image-loadermodule from next.config, and port Next'sgetWidths/generateImgAttrsinto the shim<img>; misconfiguration now fails loudlyCloses #2699.
Why
vinext does not wrap Next's
<Image>.public-shim-map.jsonreplacesnext/imagewholesale withshims/image.tsx. That means none of Next's build-time image machinery carries over automatically, and anext.configkey only works if the shim re-implements it.Upstream implements
loaderFileas a bundler alias:create-compiler-aliases.tsswapsnext/dist/shared/lib/image-loaderfor the user's file. There is no such module left to alias here, so the option was parsed and then dropped. A remotefillimage landed in the plain-<img>branch and rendered<img src="{original}" data-nimg="fill">with no optimization at all.The second defect is independent of config. The
loaderprop path invoked the loader exactly once, asloader({ src, width: imgWidth ?? 0, quality }). Afillimage has nowidth, soimgWidthisundefinedand the?? 0converted "unknown width" into a concrete but wrong0, which flowed straight into user loader code. That path also never built asrcSet.loaderFileis resolved and existence-checked at config time; an unsupportedloadervalue throwsfillpassesundefined, which maps to thedeviceSizesladder withwdescriptorsgetImagePropsmust agree with what<Image>rendersgenerateImgAttrsWhat changed
images.loaderFileset/_next/imageusedloaderFilepath does not existloaderFilemodule has no default exportimages.loader: "custom", noloaderFile, noloaderprop/_next/imageloader-prop errorimages.loader: "imgix"withloaderFilenext/legacy/imageupstreamimages.loader: "imgix"alone, noloaderFile/_next/imageimages.loader: "custom", noloaderFile,unoptimizedplaceholder="blur"getImagePropsqualityomitted75undefinedand applies its own defaultloaderprop, fixed widthsrcSetsrcSetwith1x/2xcandidates;srcis the largest candidateloaderprop,fillwidth: 0, nosrcSetdeviceSizesentry,wdescriptors,sizesdefaults to100vwgetImagePropswith a loaderwidth: 0defect,srcSet: undefined/_next/imagepath is byte-identicalRemote-image optimization through
/_next/imageis unchanged and still unsupported. That path stays reserved for a separate change.Maintainer review path
packages/vinext/src/shims/image.tsxforgetWidths/generateImgAttrsand the loader branch, which is where both defects lived.packages/vinext/src/image/image-loader-virtual.tsfor the generated module and its three states.packages/vinext/src/config/next-config.ts(resolveImageLoaderFile) for path resolution and validation.packages/vinext/src/index.tsfor theresolveId/loadwiring.tests/image-loader-plugin.test.tsfor proof that a real next.config reaches the generated module.Validation
getWidthsandgenerateImgAttrswere checked line by line against the vendored.refs/nextjs-v16.2.6/packages/next/src/shared/lib/get-img-props.ts, including thewvsxdescriptor rule, thewidths[last]choice forsrc, the!sizes && kind === "w"default to100vw, and the attribute ordering comment about Safari prefetchingsrcbeforesrcSetlands.tests/image-loader-config.test.ts: codegen across the three states, plus three tests that write the generated source to disk andimport()it, so quoting and escaping are verified by execution rather than string matching.tests/image-loader-plugin.test.ts: resolves and loadsvirtual:vinext-image-loaderthrough a real Vite server with a realnext.config.mjs, then serves throwaway projects from a dev server so the configured-loader branches run against the module the plugin actually generated. Three of those cases fail against the pre-review commit.getImagePropsregression tests assertingfillnever sendswidth: 0and produces a device-sizesrcSet.Commands and extended results
Neither unit failure is related to this change:
tests/commonjs-loader.test.ts > delegates packages with native addons to Node's loaderglobs for a compiledbetter_sqlite3.nodeaddon that was never built in this working tree. Confirmed failing onmainas well.tests/dynamic-requests-build.test.ts > serves guarded fully dynamic requests in pages and route handlers during developmenttimes out under suite parallelism and passes in isolation (43/43).Risk / compatibility
loader,srcis now the largest candidate width rather than the declared width, and asrcSetis emitted. This is upstream behaviour. Two existing tests asserted the previous output and are updated with the derivation of the expected values.images.loaderandimages.loaderFile. Previously these were accepted by the type and ignored, so no working configuration breaks. Configurations that were silently broken now fail at config time, which is the intent./_next/imagepath produces identical output.getImageWidthsis preserved and reused rather than reimplemented, so the fixed-widthx-descriptor behaviour is untouched.Non-goals
/_next/imageoptimizer.parseImageParamsstill accepts only path-relative URLs, and the SSRF surface that opens deserves its own change.sizes-lessfillcase for the built-in loader, which is next/image: fixed-size images emit w-descriptor srcSet without sizes (Next.js emits 1x/2x) #1966 and shares this code but has a wider blast radius.imgix,cloudinary,akamai). They exist only innext/legacy/imageupstream and are now rejected rather than partially honoured.overrideSrcprop is still not applied in the loader branch. Pre-existing, unrelated, left alone to keep the diff reviewable.References
.refs/nextjs-v16.2.6/packages/next/src/shared/lib/get-img-props.tsgetWidths/generateImgAttrs.refs/nextjs-v16.2.6/packages/next/src/build/create-compiler-aliases.tsloaderFilealiasing this replaces.refs/nextjs-v16.2.6/packages/next/src/server/config.tsloaderFilevalidation and error messagessrcSetbehaviour in the same file, deliberately out of scopeNote on a separate parity gap
While verifying widths I found an unrelated pre-existing divergence: vinext defaults
images.imageSizesto[16, 32, 48, 64, 96, 128, 256, 384]inindex.tsandshims/image.tsx, but upstreamimageConfigDefaultis[32, 48, 64, 96, 128, 256, 384]with no16. That extra entry changesallSizesand therefore srcSet candidate selection. Not touched here; happy to open a separate issue.