fix(image): emit preload hints for priority images#1266
Conversation
Priority images currently only render eager loading and high fetch priority. That misses the resource hint Next.js emits, so the browser cannot discover the optimized image until it reaches the img element. The image shim now calls ReactDOM.preload with the final optimized image URL, srcset, sizes, and fetch priority metadata for priority and preload images. Focused image component coverage asserts the SSR preload hint and the modern preload prop behaviour.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Clean implementation that matches Next.js behavior well. Verified against Next.js source (get-img-props.ts and image-component.tsx on canary). Tests pass.
What I checked:
-
loadingderivation matches Next.js. In Next.js:isLazy = !priority && !preload && (loading === 'lazy' || typeof loading === 'undefined'), thenloadingFinal = isLazy ? 'lazy' : loading. The vinext equivalentimageLoading = priority ? 'eager' : shouldPreload ? loading : (loading ?? 'lazy')produces identical results for all combinations ofpriority,preload, andloading. -
ReactDOM.preloadcall structure matches Next.js. Next.js passesas: 'image',imageSrcSet,imageSizes, andfetchPriority— vinext does the same. Next.js additionally passescrossOriginandreferrerPolicy, but those props aren't supported on vinext'sImagecomponent at all, so that's a pre-existing gap. -
preloadprop is destructured out of the component props, so it won't leak through...restto the DOM. -
getImagePropscorrectly computesshouldPreloadbut only uses it for theloadingderivation (no preload hint emission), which is correct sincegetImagePropsreturns props, not JSX. -
No duplicate preload calls — the remote URL paths return early, so the local image preload at the bottom of the function is only reached for local images.
-
All 53 tests pass, including the two new ones.
Two minor nits below, neither blocking.
| fetchPriority?: ReactDOM.PreloadOptions["fetchPriority"]; | ||
| }): void { | ||
| if (!input.shouldPreload) return; | ||
| if (typeof ReactDOM.preload !== "function") return; |
There was a problem hiding this comment.
Nit: The typeof ReactDOM.preload !== 'function' guard is good, but in Next.js the ImagePreload component also passes crossOrigin and referrerPolicy to the preload options (since those affect CORS and the browser's preload matching). vinext's Image component doesn't accept these props today, so nothing to fix here, but worth noting as a follow-up gap — if crossOrigin or referrerPolicy support is added to the Image component later, the preload call should be updated to pass them too.
| expect(html).toContain('<link rel="preload"'); | ||
| expect(html).toContain('as="image"'); | ||
| expect(html).toContain(`imageSrcSet="${optUrlHtml("/hero-preload.png", 640)} 640w`); | ||
| expect(html).not.toContain('loading="lazy"'); |
There was a problem hiding this comment.
Nit: This assertion confirms loading="lazy" is absent, which is correct (when preload=true without explicit loading, both Next.js and vinext omit the loading attribute entirely). Consider also asserting the positive case — that loading is not rendered at all — to make the intent clearer and catch regressions where someone might accidentally default to loading="eager" for preload-only images:
| expect(html).not.toContain('loading="lazy"'); | |
| expect(html).not.toContain('loading="lazy"'); | |
| expect(html).not.toContain('loading="eager"'); |
|
Review submitted with approval. Here's a summary of what I found: Verdict: Approved The PR correctly adds
Two minor nits posted:
|
Overview
next/imagenow callsReactDOM.preloadwith the final image URL, srcset, sizes, and fetch priority metadata whenpriorityorpreloadis set.packages/vinext/src/shims/image.tsx,tests/image-component.test.tsWhy
Next.js treats image preloading as a resource-discovery contract, not just an
<img>loading-mode tweak.priorityis now a deprecated alias forpreload, and both paths cause Next.js to emit an image preload hint. Vinext already translatedpriorityto eager loading and high fetch priority, but it did not emit the preload resource hint, so the optimized image stayed undiscoverable until the parser reached the<img>.ReactDOM.preloadforpriorityandpreload.optimizedSrc,srcSet, and computedsizes.priorityremains supported, while currentpreloadsemantics should also work.preload?: booleanand avoids default lazy loading when onlypreloadis set.What changed
<Image priority /><img>only.<link rel="preload" as="image">via React DOM and keeps eager/high-priority<img>.<Image preload />restand no preload hint was emitted./_vinext/image?...candidates.Maintainer review path
packages/vinext/src/shims/image.tsx: reviewpreloadImageResource, theshouldPreload/imageLoadingderivation, and the local optimized image branch whereoptimizedSrc,srcSet, andsizesare known.tests/image-component.test.ts: review the SSR assertions forpriorityand the currentpreloadprop.Validation
vp test run tests/image-component.test.ts -t "preload|priority"vp test run tests/image-component.test.tsvp check tests/image-component.test.tsvp checkRisk / compatibility
preloadprop and preserves existingprioritybehaviour.preloadwhen available, matching current Next.js. The helper no-ops ifReactDOM.preloadis unavailable.@unpic/reactare preloaded by their source URL because vinext does not currently expose Unpic-generated srcset data before rendering. Local optimized images get exact optimized URL/srcset coverage.Non-goals
@unpic/react.priority,preload, andloading="lazy"combinations.References
ImagePreloadsourceReactDOM.preload/<link rel="preload" as="image">semantics fornext/image.getImgPropsmetapreloadand deprecatedpriorityare both accepted image props.meta.preloadis computed as `preload