fix: support experimental.outputHashSalt config and NEXT_HASH_SALT env var#978
Conversation
…v var (cloudflare#754) Add support for Next.js experimental.outputHashSalt config option and NEXT_HASH_SALT environment variable. When set, the salt is mixed into content-addressed build output filenames via Rollup/Rolldown's output.hashSalt option, producing different hashes without modifying source code — useful for cache-busting after CDN poisoning. Config resolution: concatenates config salt + env var salt (matching Next.js behavior: config first, then env). When neither is set, no salt is applied and behavior is unchanged.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: experimental.outputHashSalt + NEXT_HASH_SALT support
The config resolution logic and tests look correct and match Next.js behavior. However, there is a significant issue with how the salt is injected into the build pipeline.
Core issue: output.hashSalt is not a Rollup or Rolldown option
The PR injects hashSalt into the Rollup/Rolldown output config object:
return { ...base, hashSalt };But hashSalt is a webpack output option, not a Rollup or Rolldown one. I searched through the Rollup docs, the Rolldown source, and all type definitions in node_modules — neither bundler recognizes output.hashSalt. The property will be silently ignored, meaning the salt has no actual effect on output filenames.
In Next.js, this works because Next.js uses webpack, which natively supports output.hashSalt to mix a salt into content hashes. In Vite/Rollup/Rolldown, the hash is content-based and there is no built-in mechanism to salt it.
To actually implement hash salting in Vite, you would likely need to either:
- Use Rolldown's
output.hashCharactersor a customhashConsingapproach (if available in Vite 8+) - Prepend/append the salt to chunk content before hashing via a plugin hook like
renderChunk - Use Vite's
build.rolldownOptions.output.entryFileNames/chunkFileNames/assetFileNameswith a function that incorporates the salt into the[hash]somehow
This needs investigation into what Vite 7 (Rollup) and Vite 8 (Rolldown) actually support. The config resolution part of this PR is good, but the build integration needs to actually change output hashes.
Minor issues
-
The
nullconfig path (line 512) doesn't concatenate withexperimental.outputHashSalt— but that's correct sinceconfigis null, there's no experimental config to read. Just noting this for completeness. -
The test cleanup pattern captures
OLD_ENVat module parse time, which is fine for serial test execution but could be fragile if tests run in parallel within the same process. This is the same pattern used elsewhere in the file, so it's consistent.
| const base = | ||
| viteMajorVersion >= 8 ? { codeSplitting: clientCodeSplittingConfig } : clientOutputConfig; | ||
| if (hashSalt) { | ||
| return { ...base, hashSalt }; |
There was a problem hiding this comment.
output.hashSalt is not a recognized Rollup or Rolldown output option. In Next.js this works because webpack natively supports output.hashSalt to mix a salt string into content hashes. Neither Rollup nor Rolldown has this feature — the property will be silently ignored and the salt will have no effect on output filenames.
To actually implement this, you'd need a different approach. Some options:
- A
renderChunkplugin hook that prepends the salt to chunk code before hashing (though this changes the actual content, not just the hash) - A
generateBundlehook that renames files with salted hashes - Investigate whether Rolldown has any hash customization options coming
At minimum, this needs a test that actually verifies output filenames change when a salt is configured (an integration/build test, not just a unit test on config resolution).
| function getClientOutputConfigForVite(viteMajorVersion: number, hashSalt?: string) { | ||
| const base = | ||
| viteMajorVersion >= 8 ? { codeSplitting: clientCodeSplittingConfig } : clientOutputConfig; | ||
| if (hashSalt) { |
There was a problem hiding this comment.
Nit: the truthiness check if (hashSalt) means an empty string "" will skip adding hashSalt to the output config. This is the correct behavior (matches the intent from next-config.ts where empty string means "no salt"), but it's worth noting that this relies on the empty-string convention from the config layer.
| // Resolve hashSalt from experimental.outputHashSalt config + NEXT_HASH_SALT env var. | ||
| // Next.js concatenates them: config value first, then env var. | ||
| const configOutputHashSalt = experimental?.outputHashSalt as string | undefined; | ||
| const hashSalt = (configOutputHashSalt ?? "") + (process.env.NEXT_HASH_SALT ?? ""); |
There was a problem hiding this comment.
The concatenation logic matches Next.js exactly ((result.experimental.outputHashSalt ?? '') + process.env.NEXT_HASH_SALT in packages/next/src/server/config.ts). This part is correct.
Note: Next.js does the concatenation inside config.ts and then stores the result back into experimental.outputHashSalt. Here, vinext stores it as a separate hashSalt field on ResolvedNextConfig, which is a cleaner approach.
|
Review submitted successfully. Here's a summary of the review: Requested changes on PR #978. Core finding: The config resolution logic (parsing The PR correctly reads and stores the salt, but the mechanism for applying it to the build output needs to be reworked using Vite-compatible approaches (e.g., plugin hooks like |
The output.hashSalt option is webpack-specific and is silently ignored by Rollup/Rolldown. Use the augmentChunkHash plugin hook instead, which is supported by both bundlers and actually affects chunk content hashes. Changes: - Remove hashSalt parameter from getClientOutputConfigForVite() - Add vinext:hash-salt plugin with augmentChunkHash hook - Add experimental.outputHashSalt to CONFIG_SUPPORT map
|
/bigbonk review |
There was a problem hiding this comment.
Review: experimental.outputHashSalt + NEXT_HASH_SALT support (post-revision)
The previous review's core concern (output.hashSalt being a webpack-only option) has been addressed — the PR now uses augmentChunkHash, which is a valid Rolldown plugin hook. I verified it exists in rolldown@1.0.0-rc.15 types with the signature (this: PluginContext, chunk: RenderedChunk) => string | void.
The config resolution logic is correct and matches Next.js's concatenation order. Tests are solid and cover the important cases. The augmentChunkHash approach is the right mechanism for Vite/Rolldown. Two items below — one is a scope gap worth documenting, the other is a minor nit.
Overall this looks good to land.
| if (salt) { | ||
| return salt; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Scope gap: augmentChunkHash only affects JS chunks, not CSS or static assets.
In Next.js, outputHashSalt affects three things:
- JS chunk hashes — via webpack
output.hashSalt - CSS chunk hashes — via webpack
output.hashSalt(same mechanism) - Image hashes — via a separate manual salt prepend in
next-image-loader
The Next.js test suite (test/production/app-dir/hash-salt/hash-salt.test.ts) explicitly verifies that changing the salt changes filenames for JS chunks, images, and CSS.
augmentChunkHash in Rolldown is documented as "called for each Rolldown output chunk" and is the right hook for JS chunks. Whether it also affects CSS chunks depends on whether Rolldown/Vite emits CSS as chunks (which Vite 8 with cssCodeSplit may do) or as assets (where it would not apply).
This isn't a blocker — JS chunk salting is the primary use case and this PR delivers that correctly. But it would be good to:
- Add a brief comment noting the CSS/asset gap for future work
- File a follow-up issue if full parity is desired
Also, the comment on line 3212 says "supported by both Rollup (via crypto hash.update()) and Rolldown (via xxhash_base64_url)" — I'd soften this since the Rollup compatibility claim is hard to verify from the current deps (this project runs on Vite 8 / Rolldown, not Rollup). Simpler to just say "supported by Rolldown".
| apply: "build", | ||
| augmentChunkHash() { | ||
| // Only apply to client environment; SSR/RSC don't use content hashing | ||
| if (this.environment?.name !== "client") return; |
There was a problem hiding this comment.
Nit: the this.environment?.name pattern is consistent with other hooks in this file, but for augmentChunkHash specifically, the hook only runs during build (enforced by apply: "build"), and in a multi-environment build, each environment's plugins run independently. So the guard is correct — just confirming this is doing the right thing.
One thing to double-check: does the RSC plugin's multi-environment build pipeline call augmentChunkHash once per environment, with this.environment correctly set? If for some reason this.environment is undefined during the build (e.g., fallback environment), the hook would silently return undefined for all environments including client. Given that other hooks in this file use the same ?.name pattern, this is likely fine, but worth a sanity check if you can test with an actual build.
|
Review submitted. Here's a summary: Previous concern resolved: The earlier review flagged that Key findings:
|
- Simplify comment about augmentChunkHash (just Rolldown, not Rollup) - Add scope-gap note about CSS/static asset hashing not being salted
|
/bigbonk review |
There was a problem hiding this comment.
Review: experimental.outputHashSalt + NEXT_HASH_SALT support
This is clean and correct. The config resolution matches Next.js's concatenation semantics, the augmentChunkHash hook is the right Rolldown mechanism, and the tests cover the important paths. Two previous review rounds already addressed the main concern (switching from the invalid output.hashSalt to augmentChunkHash), and that's been resolved well.
All 73 tests pass (6 new). One minor nit below, otherwise LGTM.
| }, | ||
| "experimental.outputHashSalt": { | ||
| status: "supported", | ||
| detail: "salt mixed into output content hashes for cache-busting", |
There was a problem hiding this comment.
Nit: this says "output content hashes" which implies all output files. Since the implementation only salts JS chunk hashes (via augmentChunkHash), this detail could be more precise to avoid misleading vinext check output.
| detail: "salt mixed into output content hashes for cache-busting", | |
| detail: "salt mixed into JS chunk content hashes for cache-busting (CSS/asset hashes not yet salted)", |
|
Review submitted. Here's a summary: Approved with one minor nit. The PR correctly implements
|
2 participants
Fixes #754
Summary
Test plan