presets(vercel): support immutable static files - #4432
Conversation
Co-Authored-By: Rihan <me@file.properties>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesVercel immutable asset flow
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
d1f4dab to
b6ab335
Compare
b6ab335 to
4472a85
Compare
9bc6c67 to
d2f3916
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/presets/vercel/preset.ts (1)
29-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winParse the environment flag instead of coercing presence to boolean.
!!converts"false"and"0"intotrue, so users cannot disable immutable assets through the environment variable.
src/presets/vercel/preset.ts#L29-L29: explicitly parseVERCEL_IMMUTABLE_STATIC_FILES_ENABLED.src/presets/vercel/preset.ts#L142-L142: apply the same parsing logic tovercel-static, preferably through a shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/vercel/preset.ts` at line 29, Parse VERCEL_IMMUTABLE_STATIC_FILES_ENABLED as an actual environment boolean instead of coercing its presence with !!, treating values such as "false" and "0" as disabled. Apply the same parsing logic to the vercel-static configuration at src/presets/vercel/preset.ts lines 29-29 and 142-142, preferably via a shared helper.
🧹 Nitpick comments (1)
test/presets/vercel.test.ts (1)
467-468: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert manifest contents, not only its shape.
hashes: {}would satisfy these assertions, so the test would not catch a broken glob or URL mapping. Assert at least one expected asset entry and its generated URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/presets/vercel.test.ts` around lines 467 - 468, Strengthen the manifest assertions in the test around manifest.version and manifest.hashes by verifying that hashes contains at least one expected asset entry and that the entry has the correct generated URL. Keep the existing version assertion and object-shape check while ensuring broken glob or URL mapping behavior fails the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/presets/vercel.test.ts`:
- Line 462: The buildAssetsDir assertion in the Vercel preset test is hard-coded
despite immutableDir() incorporating VERCEL_HASH_SALT. Update the expectation to
derive the immutable path using the active VERCEL_HASH_SALT value, or isolate
the test by clearing that environment variable and restoring it afterward.
---
Outside diff comments:
In `@src/presets/vercel/preset.ts`:
- Line 29: Parse VERCEL_IMMUTABLE_STATIC_FILES_ENABLED as an actual environment
boolean instead of coercing its presence with !!, treating values such as
"false" and "0" as disabled. Apply the same parsing logic to the vercel-static
configuration at src/presets/vercel/preset.ts lines 29-29 and 142-142,
preferably via a shared helper.
---
Nitpick comments:
In `@test/presets/vercel.test.ts`:
- Around line 467-468: Strengthen the manifest assertions in the test around
manifest.version and manifest.hashes by verifying that hashes contains at least
one expected asset entry and that the entry has the correct generated URL. Keep
the existing version assertion and object-shape check while ensuring broken glob
or URL mapping behavior fails the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba51474d-6bcd-4384-bae0-46ff51f619a5
📒 Files selected for processing (4)
src/presets/vercel/immutable.tssrc/presets/vercel/preset.tstest/fixture/nitro.config.tstest/presets/vercel.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/presets/vercel.test.ts (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
try/finallyto guarantee environment restoration.If the build/setup phase fails, the environment variable won't be restored, potentially leaking into cleanup hooks or other test suites in the same file. Consider wrapping the setup phase (between the override and the restoration) in a
try/finallyblock to ensureprocess.env.VERCEL_HASH_SALTis always restored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/presets/vercel.test.ts` around lines 46 - 50, Wrap the test setup and execution between the VERCEL_HASH_SALT override and restoration in a try/finally block, ensuring the existing restoration logic runs even when setup or assertions fail. Keep the undefined case deleting process.env.VERCEL_HASH_SALT and the defined case restoring its previous value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/presets/vercel.test.ts`:
- Around line 46-50: Wrap the test setup and execution between the
VERCEL_HASH_SALT override and restoration in a try/finally block, ensuring the
existing restoration logic runs even when setup or assertions fail. Keep the
undefined case deleting process.env.VERCEL_HASH_SALT and the defined case
restoring its previous value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 888ef528-c2dd-417d-a99c-a4b161934e0e
📒 Files selected for processing (3)
src/build/vite/env.tssrc/build/vite/plugin.tstest/presets/vercel.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/build/vite/env.ts
- src/build/vite/plugin.ts
| // `useLongerAssetHashes` in `plugin.ts`) so a `?url` asset import | ||
| // resolves to the same filename on both sides. | ||
| ...(buildAssetsDir | ||
| ? { assetFileNames: `${buildAssetsDir}/[name]-[hash:16][extname]` } |
There was a problem hiding this comment.
TODO: We are still blindly overriding assetFileNames for server envs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/presets/vercel/immutable.ts (1)
60-75: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSort the glob results before serializing the manifest.
tinyglobbydoes not guarantee file order, soObject.fromEntries(files.map(...))can produce differentimmutable.jsonbytes for the same asset set. Sortingfilesfirst keeps the manifest stable and avoids cache churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/vercel/immutable.ts` around lines 60 - 75, Sort the files array returned by glob before mapping it into the hashes object in the immutable manifest generation flow. Update the files processing near ImmutableManifest so identical asset sets always produce deterministic entry ordering and stable immutable.json output.
🧹 Nitpick comments (1)
src/build/vite/plugin.ts (1)
493-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an options object for the helper configuration.
useLongerAssetHashes(build, isRolldown, assetsDir)has multiple positional configuration arguments. Use{ isRolldown, assetsDir }as the second parameter to make call sites clearer and avoid future signature churn.As per coding guidelines, multi-argument functions should use an options object as the second parameter.
Suggested shape
-export function useLongerAssetHashes(build, isRolldown, assetsDir) +export function useLongerAssetHashes( + build, + { isRolldown, assetsDir }: { isRolldown: boolean | undefined; assetsDir: string } +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/build/vite/plugin.ts` around lines 493 - 527, Update useLongerAssetHashes to accept a single options object containing isRolldown and assetsDir instead of separate positional arguments, and adjust every call site to pass those named properties while preserving the existing hash-upgrade behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/presets/vercel/immutable.ts`:
- Around line 60-75: Sort the files array returned by glob before mapping it
into the hashes object in the immutable manifest generation flow. Update the
files processing near ImmutableManifest so identical asset sets always produce
deterministic entry ordering and stable immutable.json output.
---
Nitpick comments:
In `@src/build/vite/plugin.ts`:
- Around line 493-527: Update useLongerAssetHashes to accept a single options
object containing isRolldown and assetsDir instead of separate positional
arguments, and adjust every call site to pass those named properties while
preserving the existing hash-upgrade behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 717b4f1c-51cd-4e18-9da6-c50ea050707a
📒 Files selected for processing (5)
docs/2.deploy/20.providers/vercel.mdsrc/build/vite/plugin.tssrc/presets/vercel/immutable.tssrc/presets/vercel/preset.tssrc/presets/vercel/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/presets/vercel/types.ts
- docs/2.deploy/20.providers/vercel.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/build/vite/plugin.ts (1)
511-515: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an options object for helper configuration.
useLongerAssetHashesadds configuration as positional parameters instead of using an options object as the second parameter, contrary to the repository guideline. Refactor the signature and its call sites before adding further options.Suggested signature
export function useLongerAssetHashes( build: NonNullable<EnvironmentOptions["build"]>, - isRolldown: boolean | undefined, - assetsDir: string, - opts?: { assetsOnly?: boolean } + { + isRolldown, + assetsDir, + assetsOnly = false, + }: { + isRolldown?: boolean; + assetsDir: string; + assetsOnly?: boolean; + } ): void {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/build/vite/plugin.ts` around lines 511 - 515, Refactor useLongerAssetHashes to accept a single options object for all helper configuration, including the existing isRolldown, assetsDir, and optional assetsOnly values. Update every call site to pass the equivalent named properties, preserving current behavior and parameter values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/build/vite/plugin.ts`:
- Around line 505-510: The output-options handling around the client/SSR
configuration must preserve explicitly configured entryFileNames,
chunkFileNames, and assetFileNames, including patterns containing [hash]. Only
populate fields that are undefined; move any longer-hash defaulting before
custom output patterns are applied, and update both affected branches without
rewriting user or plugin values.
---
Nitpick comments:
In `@src/build/vite/plugin.ts`:
- Around line 511-515: Refactor useLongerAssetHashes to accept a single options
object for all helper configuration, including the existing isRolldown,
assetsDir, and optional assetsOnly values. Update every call site to pass the
equivalent named properties, preserving current behavior and parameter values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0446596e-868a-480f-99ef-8b7a08e80e4d
📒 Files selected for processing (1)
src/build/vite/plugin.ts
- disable immutable static files (with a warning) on a non-root baseURL, since they must be served from the reserved `/_vercel/immutable/` path - warn instead of silently writing an empty manifest when no assets are emitted under `buildAssetsDir` - normalize `buildAssetsDir` in config and when deriving the immutable dir - correct manifest comments and docs: values are file paths (explicitly allowed by the spec) and `VERCEL_HASH_SALT` is factored into the asset paths rather than the content hashes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Draft docs:
https://nitrobuild-git-feat-vercel-immutable-nitrojs.vercel.app/deploy/providers/vercel#immutable-static-files
Preview package for e2e testing:
Then opt-in via
vercel.immutableStaticFiles: truenitro config orNITRO_VERCEL_IMMUTABLE_STATIC_FILES_ENABLEDLocal testing in examples:
v2 backport: #4434