fix(image): emit static imports as managed assets - #2040
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(image): emit static imports as managed assets
I reviewed the diff across index.ts, static-file-cache.ts, image.tsx, and the tests. Ran the relevant suites locally — static-image-emission, image-component, image-imports, static-file-cache all pass (4 + 59 + 26 + 27), and vp check is clean on the changed source files. The implementation is solid and well-targeted. No blocking issues found.
What's correct and well done
- Static media emission writes to
dist/client/_next/static/media/— exactly where both the Node prod server (resolveAppRouterAssetPath) and Cloudflare static serving read from. Verified the on-disk path matches the emitted URL for the default,assetPrefix, and path-prefix layouts (thebasePath/assetPrefixtest exercises this). - Server-only RSC imports are handled:
staticImageImportsByModuleis shared across the RSC/SSR/client environments (single plugin instance, keyed by module id), so the clientwriteBundlesees RSC-collected imports and emits them. The build-order reasoning in the PR body checks out. - ETag scoping (
etagFromFilenameHash) correctly restricts the dot-delimited 8-hex form to_next/static/media/, so_next/static/config.deadbeef.jsonno longer gets a bogusW/"deadbeef". The dash branch still handles Vite hashes first; names likemy-icon.0123abcd.pngfall through to the dot branch (dash suffix exceeds the 6–12 length cap). Good coverage of both the positive and negative cases. isSvgUrlis a genuine correctness/security fix:.endsWith(".svg")missed managed SVG sources carrying?dpl=..., which would have routed them to the optimizer instead of skipping. Parsing the pathname matches Next's "SVG is unoptimized by default" behavior.
Minor (non-blocking)
1. CodeQL flagged .replace("&", "&") in tests/image-component.test.ts:233-234 — only replaces the first &. It happens to be a no-op here because the test src (...icon.0123abcd.svg?dpl=deployment-1) contains no &, so the assertions are correct today. But the intent is HTML-escaping, and it would silently break if anyone added a multi-param query. Suggest .replaceAll("&", "&") (or a real escape helper) to make the intent explicit and satisfy CodeQL.
2. buildStart clears staticImageAssets per-environment. In a multi-env production build, the client buildStart wipes the RSC-populated entries before client load repopulates. It's not a bug — writeBundle falls back to createStaticImageAsset(imagePath) reading from disk, and the server-component test confirms emission works — but the asset cache is effectively bypassed for server-only images (redundant re-reads/re-hashes at write time). Optional: skip the staticImageAssets.clear() (rely on watchChange for invalidation) or only clear in the first environment, if you want the cache to actually pay off.
3. Map key-space mismatch in watchChange(id). It deletes id from both staticImageAssets (keyed by image path) and staticImageImportsByModule (keyed by importer module id). For an image-file change, id is the image path, so the staticImageImportsByModule.delete(id) is a no-op (and vice-versa for a source-module change). Harmless given the disk-read fallback, but a brief comment noting the two key spaces would help future readers.
None of these block merge. Tests are thorough (App + Pages routers, server + client components, basePath/assetPrefix/deploymentId, immutable caching + 304 parity, and the inline-threshold-preserved control case).
|
Review posted on PR #2040. SummaryI reviewed PR #2040 ( Verified correct:
Minor (non-blocking) notes raised:
Tests are thorough, covering both routers, server/client components, basePath/assetPrefix/deploymentId, immutable caching + 304 parity, and a control case proving ordinary |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(image): emit static imports as managed assets
Re-reviewed the diff (index.ts, static-file-cache.ts, image.tsx, tests) and re-ran the suites locally — static-image-emission + static-file-cache pass (31 tests). The implementation is solid and the failing wave2 contract is correctly addressed. No blocking issues.
Verified correct
- Cross-environment URL stability. The
?vinext-image-urlvirtual module returns a deterministic, content-hashed Next-shaped URL (renderVinextBuiltUrl(...)). Because RSC, SSR, and client environments each runloadindependently but compute the same SHA-256 content hash,StaticImageData.srcis now identical across environments — which is exactly the divergence the wave2 failure exposed (RSC inlining a data URL while client emitted a file). - On-disk / URL parity.
writeBundlewrites topath.join(clientOutDir, resolveAssetsDir(assetPrefix), asset.fileName), and the rendered URL is built from the sameresolveAssetsDir(...) + "/" + asset.fileName. ThebasePath/assetPrefix/deploymentIdtest confirms the path-prefix layout (cdn/_next/static/media/...) lands where the prod server and Cloudflare ASSETS binding read from. No double-prefixing. - Server-only RSC emission.
staticImageImportsByModuleis keyed by importer module id on the single shared plugin instance, so the clientwriteBundlesees RSC-collected imports and emits them even when no client module imports the image. - ETag scoping.
etagFromFilenameHashrestricts the dot-delimited 8-hex form to_next/static/media/. Verified the dash branch runs first and falls through for dashed media names (my-icon.0123abcd→ dash suffixname.0123abcdexceeds the 6–12 cap and contains a., failing the charclass), and that arbitrary files like_next/static/config.deadbeef.jsonno longer receive a bogusW/"deadbeef". Both positive and negative cases are covered. isSvgUrlfix. Parsing the pathname instead of.endsWith(".svg")correctly skips the optimizer for deployment-tagged managed SVGs (...icon.HASH.svg?dpl=...), matching Next's "SVG is unoptimized by default" behavior. The earlier CodeQL.replace("&", ...)finding was already resolved with.replaceAllin320213a.
Minor (non-blocking)
buildStartclearsstaticImageAssetsper environment. In the multi-env build, the clientbuildStartwipes RSC-populated entries beforewriteBundleruns, sowriteBundlefalls back tocreateStaticImageAsset(imagePath)re-reading/re-hashing from disk for server-only images. Not a bug (the server-component test confirms emission), just a redundant re-read — the asset cache effectively never pays off for server-only images. Optional: skipstaticImageAssets.clear()and rely onwatchChangefor invalidation.watchChange(id)mixes two key spaces. It deletesidfrom bothstaticImageAssets(keyed by image path) andstaticImageImportsByModule(keyed by importer module id); only one delete can ever hit per call. Harmless given the disk-read fallback and the importer's ownwatchChangere-runningtransform, but a one-line comment noting the two key spaces would help future readers.media/directory shared with Vite-emitted assets. Vite's own asset output (createClientAssetFileNames) also writes to<assetsDir>/media/. Collision is effectively impossible (Vite uses dash hashesname-HASH.ext, vinext uses dot hashesname.HASH.ext), but worth being aware of if the asset naming scheme ever changes.
Test coverage is thorough: both routers, server + client components, basePath/assetPrefix/deploymentId, watch rebuilds (changed image rehash + removed-import cleanup), immutable caching + 304 conditional parity, and a control case proving ordinary ?url imports keep the user's inline threshold. Nothing blocks merge.
|
Review posted on PR #2040. SummaryI re-reviewed PR #2040 ( Verified correct:
Minor (non-blocking) notes raised:
Test coverage is comprehensive across both routers, server/client components, basePath/assetPrefix/deploymentId, watch rebuilds, immutable caching + 304 parity, and the inline-threshold control case. |
vinext parity: static image import emission
Source evidence
/tmp/vinext-e2e-27514800656-backlog-wave2.md2751480065610b5086f4d96a7f2054cbc9c6c239aa16a1d6dd8(June 14, 2026)test/e2e/app-dir/next-image/next-image.test.tsshould render images nested under page dir on /nested routeshould render images on /client routeurl=was/_next/static/media/test.HASH.pngand whose selected width was828; received an optimizer URL whoseurl=was a largedata:image/png;base64,...and whose width was400.Upstream and bundler semantics
next-image-loaderalways emits imported image content and returnsStaticImageDatawith a URL under/static/media/[name].[hash:8].[ext]; it does not apply a general inline threshold to static image imports.asset/resourcewhere managed asset URLs are required, specifically to avoid data-URI inlining.build.assetsInlineLimitindependently in each build environment. In App Router builds, RSC can therefore inline a source even when the client environment emits it, producing differentStaticImageData.srcvalues across environments.Reproduction added
tests/static-image-emission.test.tsproduction-builds small real fixtures withbuild.assetsInlineLimit: 100_000so the images would inline without an explicit framework override.Coverage:
dist/client/.../_next/static/media/with original bytes.srcandsrcSetpoint to the same managed media URL and never containdata:image.?urlimport remains a data URL, proving the user's inline threshold is preserved for unrelated assets.basePath, pathassetPrefix, anddeploymentIdURL/on-disk behavior.Implementation
?vinext-image-urlvirtual module plus the existing metadata module.?urlhandling.media/[name].[hash][ext],assetPrefixanddeploymentId.StaticImageDatashape are unchanged.assetsInlineLimitoverride was added; public files, CSS assets, ordinary Vite imports, and explicit user thresholds retain their existing behavior.Validation
Branch base: current
origin/mainata3d2f921520ff140a826224616df5e0db4ed0186on June 15, 2026.Passed:
vp test run tests/image-imports.test.ts tests/image-component.test.ts tests/image-optimization-parity.test.ts tests/static-image-emission.test.tsvp checkvp run vinext#buildgit diff --checkExact assertion disposition
The local production assertion corresponding to the wave2 failure now observes:
url=resolving to a managed/_next/static/media/<name>.<8-char-hash>.pngURL;srcSetcandidates using that managed URL;data:imagesource;The full upstream Next.js deploy suite was not rerun locally; the focused vinext-owned production fixture exercises the same failing contract without cacheComponents/PPR/resume scope.
Review follow-up — 4225b4c
All static-image review findings were addressed in commit
4225b4cfd705a12b71079e7499b7c0a384a2f606:addWatchFilefor image URL and metadata modules..svg?dpl=...sources bypass the image optimizer unless SVG optimization is explicitly allowed.If-None-Matchresponses returning 304 with no body.Review validation completed June 15, 2026:
vp test run tests/static-image-emission.test.ts tests/image-component.test.ts tests/static-file-cache.test.ts tests/serve-static.test.ts tests/image-imports.test.ts— 5 files, 151 tests passed.vp check— all 1953 files formatted; no warnings, lint errors, or type errors in 899 files.vp run vinext#build— package build completed successfully.git diff --check— clean before commit.No upstream deploy suite was started. Nothing was pushed and no PR was opened.
Independent review follow-up — managed media ETag scope
The independent P2 review finding was addressed by restricting vinext's dot-delimited eight-character image hash ETag format to files under
_next/static/media/, including path-prefixed layouts. Dash-delimited Vite asset hashes remain unchanged.Focused regression coverage now verifies:
_next/static/config.deadbeef.jsonfile does not receiveW/"deadbeef";200with the new content rather than an incorrect304;_next/static/media/photo.0123abcd.pngfiles retain the stable filename-derived ETag, immutable cache control, and matching conditional304behavior.Validation completed June 15, 2026:
vp test run tests/static-file-cache.test.ts tests/serve-static.test.ts— 2 files, 64 tests passed.vp check --fix packages/vinext/src/server/static-file-cache.ts tests/static-file-cache.test.ts tests/serve-static.test.ts— formatting completed; no warnings, lint errors, or type errors in 3 files.git diff --check— clean.