-
Notifications
You must be signed in to change notification settings - Fork 367
feat(image): support images.loaderFile and emit srcSet from custom loaders #2700
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
Open
NathanDrake2406
wants to merge
11
commits into
cloudflare:main
Choose a base branch
from
NathanDrake2406:feat/image-custom-loader
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1271179
feat(image): support images.loaderFile and custom-loader srcSet
NathanDrake2406 22efe0e
Merge branch 'main' into feat/image-custom-loader
NathanDrake2406 3707766
fix(image): preserve custom loader URL ownership
NathanDrake2406 1976cf2
fix(image): validate custom loaders before optimization is decided
NathanDrake2406 c98d303
test(image): drive configured-loader cases through the real plugin
NathanDrake2406 ff11d0b
refactor(image): expose loader state as a module export
NathanDrake2406 b4cd958
fix(image): bypass loaders for data:/blob: and order src after srcSet
NathanDrake2406 741fb3e
fix(image): merge the blur placeholder after the caller's style
NathanDrake2406 f6321d8
refactor(image): collapse duplicated shim logic and drop redundant lo…
NathanDrake2406 97eefa3
Merge branch 'main' into feat/image-custom-loader
NathanDrake2406 138a29f
Merge branch 'main' into feat/image-custom-loader
NathanDrake2406 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| /** | ||
| * Stand-in for the generated `virtual:vinext-image-loader` module. | ||
| * | ||
| * Tests import the `next/image` shim directly, without the vinext vite plugin, | ||
| * so the virtual module has no resolver. This file is aliased in place of it | ||
| * (see `WORKSPACE_SRC_ALIAS` in vite.config.ts) and mirrors what the generator | ||
| * emits when `images.loaderFile` is not configured. | ||
| * | ||
| * Tests that need a configured loader should assert on | ||
| * `generateImageLoaderModule()` output rather than trying to swap this module. | ||
| */ | ||
| export default undefined; | ||
| export const requiresLoaderProp = false; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| /** | ||
| * Code generation for the `virtual:vinext-image-loader` module, resolved by the | ||
| * vinext vite plugin from `images.loaderFile` in next.config. | ||
| * | ||
| * Next.js implements `loaderFile` as a bundler alias: the module that | ||
| * `next/image` imports for its default loader | ||
| * (`next/dist/shared/lib/image-loader`) is swapped for the user's file — see | ||
| * `create-compiler-aliases.ts` upstream. vinext replaces `next/image` wholesale | ||
| * with its own shim, so there is no upstream module left to alias. Instead the | ||
| * shim imports this virtual module unconditionally and the plugin generates | ||
| * either a re-export of the user's loader or an inert stub. | ||
| * | ||
| * Every branch emits the same two exports so the shim's unconditional import | ||
| * stays valid whether or not `loaderFile` is configured: `default` is the loader | ||
| * (or `undefined`), and `requiresLoaderProp` says whether the configuration | ||
| * demands that each `<Image>` bring its own `loader` prop. | ||
| * | ||
| * This mirrors the adapter pattern in `image/image-adapters-virtual.ts`. | ||
| */ | ||
|
|
||
| /** Public virtual module id imported by the `next/image` shim. */ | ||
| export const VIRTUAL_IMAGE_LOADER = "virtual:vinext-image-loader"; | ||
|
|
||
| /** | ||
| * Next.js's error for a `loaderFile` whose module has no default export. | ||
| * Kept verbatim so existing troubleshooting docs and searches still apply. | ||
| */ | ||
| const MISSING_DEFAULT_EXPORT_ERROR = | ||
| "images.loaderFile detected but the file is missing default export.\n" + | ||
| "Read more: https://nextjs.org/docs/messages/invalid-images-config"; | ||
|
|
||
| /** | ||
| * Generate the source of the `virtual:vinext-image-loader` module. | ||
| * | ||
| * @param images The resolved `images` block from next.config. `loaderFile` is | ||
| * expected to already be an absolute path (see `resolveImageLoaderFile`). | ||
| */ | ||
| export function generateImageLoaderModule(images?: { | ||
| loader?: "default" | "custom"; | ||
| loaderFile?: string; | ||
| }): string { | ||
| const loaderFile = images?.loaderFile; | ||
|
|
||
| // `loader: "custom"` with no `loaderFile` means every image must supply its | ||
| // own `loader` prop. Export a loader that reports the omission instead of | ||
| // silently falling back to `/_next/image`, matching upstream's `customLoader`. | ||
| if (!loaderFile && images?.loader === "custom") { | ||
| return [ | ||
| '// vinext: images.loader is "custom" with no images.loaderFile — each', | ||
| "// <Image> must pass its own `loader` prop.", | ||
| "export default function customImageLoader({ src }) {", | ||
| " throw new Error(", | ||
| " 'Image with src \"' + src + '\" is missing \"loader\" prop.\\n' +", | ||
| " 'Read more: https://nextjs.org/docs/messages/next-image-missing-loader',", | ||
| " );", | ||
| "}", | ||
| "", | ||
| "// Tells the shim the default export reports a misconfiguration rather than", | ||
| "// generating URLs — see image/image-loader-virtual.ts.", | ||
| "export const requiresLoaderProp = true;", | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
|
|
||
| // Nothing configured → the shim falls back to its built-in `/_next/image` | ||
| // loader. `undefined` (not `null`) so the shim's `??` fallback reads naturally. | ||
| if (!loaderFile) { | ||
| return [ | ||
| "// vinext: no images.loaderFile configured — the built-in /_next/image loader is used.", | ||
| "export default undefined;", | ||
| "export const requiresLoaderProp = false;", | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
|
|
||
| return [ | ||
| "// vinext: generated from `images.loaderFile` in your next.config.", | ||
| `import * as __vinextUserImageLoaderModule from ${JSON.stringify(loaderFile)};`, | ||
| "", | ||
| "const __vinextImageLoader = __vinextUserImageLoaderModule.default;", | ||
| "", | ||
| "// Configuring loaderFile is an explicit opt-in, so a module that cannot", | ||
| "// supply a loader is a config error worth failing on immediately rather", | ||
| "// than silently falling back to the built-in optimizer — which would be", | ||
| "// indistinguishable from the loaderFile being ignored.", | ||
| "if (typeof __vinextImageLoader !== 'function') {", | ||
| ` throw new Error(${JSON.stringify(MISSING_DEFAULT_EXPORT_ERROR)});`, | ||
| "}", | ||
| "", | ||
| "export default __vinextImageLoader;", | ||
| "export const requiresLoaderProp = false;", | ||
| "", | ||
| ].join("\n"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an existing Next.js app configures
images.loaderasimgix,cloudinary, orakamaiwithoutloaderFile, this branch now throws during config resolution and prevents the app from starting. These are current valid loader values—the repository's vendored Next 16.2.7 declaration inpackages/types/next/upstream/dist/shared/lib/image-config.d.tsincludes all three inVALID_LOADERS—so the claim that they exist only fornext/legacy/imageis incorrect. Implement their URL generation or preserve a nonfatal fallback rather than rejecting valid Next.js configuration.AGENTS.md reference: AGENTS.md:L293-L297
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagreeing on this one — Next.js rejects this configuration too, so the current behavior is parity, not a regression.
VALID_LOADERSdoes list all three, but that list gates bothnext/imageandnext/legacy/image. What decides the outcome is the config validation. Next 16.2.7,dist/server/config.js:540-541:So
images.loader: "imgix"with noloaderFileand no customimages.paththrows during config resolution and prevents the app from starting — in Next.js, today. That is precisely the case this branch rejects. Note also where upstream's own error message points: thenext/legacy/imageloader-configuration docs.The implementations back that up. In 16.2.7 the named loaders exist only in
dist/client/legacy/image.js:dist/shared/lib/image-loader.js— what modernnext/imageactually uses — contains onlydefaultLoader(:13, marked__next_img_defaultat:110). There is no imgix/cloudinary/akamai code path in modernnext/imageto implement or fall back to.The one real difference: upstream permits a named loader when
images.pathis set to a custom prefix, because legacy image consumes it. vinext implements neithernext/legacy/imagenor customimages.path, so there is no configuration under which a named loader could work — which is why this rejects unconditionally and says so in the message.A non-fatal fallback is specifically what the earlier round of this review asked to remove, and for a good reason: silently serving
/_next/imagewhen the config names a CDN is the failure mode that is hardest to notice in production.Reverting would restore that. Leaving as-is unless there's a concrete case where Next.js starts an app in this configuration — happy to look at one.