fix(vite): skip early dev handler for special prefixes#3817
Conversation
dev handler for special prefixes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTightens nitroDevMiddlewarePre routing in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
commit: |
![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
🧹 Nitpick comments (1)
src/build/vite/dev.ts (1)
253-260: Logic is sound. Consider minor refactors for clarity.The three-condition check correctly narrows when the Nitro middleware runs, avoiding overlap with Vite's static asset serving. The regex patterns are appropriate for their purposes.
Optional refactor 1: The file extension check could be more readable:
- // No file extension (/src/index.ts) - !req.url!.match(/\.([a-z0-9]+)(?:[?#]|$)/i)?.[1] && + // No file extension (/src/index.ts) + !/\.([a-z0-9]+)(?:[?#]|$)/i.test(req.url!) &&This avoids the optional chaining and is clearer in intent (testing for absence rather than negating a captured group).
Optional refactor 2: The special prefix check has an unnecessary
.*:- // Special prefixes (/__vue-router/auto-routes, /@vite-plugin-layouts/, etc) - !/^\/(?:__|@).*/.test(req.url!) + // Special prefixes (/__vue-router/auto-routes, /@vite-plugin-layouts/, etc) + !/^\/(?:__|@)/.test(req.url!)The
.*doesn't affect the match since we're only checking the prefix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/build/vite/dev.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/build/vite/dev.ts (1)
src/runtime/internal/vite/dev-worker.mjs (2)
server(175-188)req(141-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-rollup (ubuntu-latest)
- GitHub Check: tests-rollup (windows-latest)
- GitHub Check: tests-rolldown (windows-latest)
🔇 Additional comments (2)
src/build/vite/dev.ts (2)
246-247: LGTM! Helpful context.The comment clearly explains the intent and provides a reference to the related Vite PR for future maintainers.
253-260: Middleware architecture is sound—no backward compatibility risks identified.The implementation uses a two-tier middleware system: a pre-middleware that conditionally routes certain requests to
nitroDevMiddleware, with a fallback middleware that catches all remaining requests. This ensures nothing is dropped:
- Static assets (extensions): Skip pre-middleware → handled by Vite → fallback runs
- Dynamic routes (no extension, browser/curl): Pre-middleware routes to Nitro
- Vite internal routes (
/__,/@): Skip pre-middleware → handled by Vite → fallback runsThe
sec-fetch-destheader detection is a standard practice to distinguish browser navigation requests, and the extension regex correctly identifies file types. The reference to vitejs/vite/pull/20866 confirms alignment with Vite's own approach. All requests proceed through at least one handler—nothing is blocked.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/build/vite/dev.ts (1)
256-257: Consider documenting the API route limitation.Since paths starting with
/__or/@are now excluded from early dev handling, API routes using these prefixes may not work as expected unless a matching file exists in the project root. Adding a brief comment about this limitation would help prevent developer confusion.For example:
// Special prefixes (/__vue-router/auto-routes, /@vite-plugin-layouts/, etc) + // Note: API routes with these prefixes require a matching file in project root !/^\/(?:__|@).*/.test(req.url!)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/build/vite/dev.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/build/vite/dev.ts (1)
src/runtime/internal/vite/dev-worker.mjs (2)
server(175-188)req(141-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-rolldown (windows-latest)
- GitHub Check: tests-rollup (windows-latest)
- GitHub Check: tests-rollup (ubuntu-latest)
🔇 Additional comments (1)
src/build/vite/dev.ts (1)
246-263: LGTM! Previous caching concern is now resolved.The unconditional
Vary: sec-fetch-destheader on line 250 correctly addresses the previous review comment about cache correctness. The extended condition logic properly guards against conflicts with Vite's virtual module prefixes (/__,/@) while maintaining support for legitimate SSR/API requests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/build/vite/dev.ts (1)
253-254: Fix the comment example (duplicate issue).The example
/src/index.tshas a.tsextension, which contradicts the "No file extension" description. This was flagged in a previous review but hasn't been addressed yet.Apply this diff:
- // No file extension (not /src/index.ts) + // No file extension (e.g., /api/users) !req.url!.match(/\.([a-z0-9]+)(?:[?#]|$)/i)?.[1] &&
🧹 Nitpick comments (1)
src/build/vite/dev.ts (1)
255-256: Consider adding a warning about API route limitations.The regex correctly excludes paths starting with
/__or/@to avoid conflicts with Vite plugins. However, the PR description mentions that API routes with these prefixes require special handling (a file must exist in the project root). Consider adding a comment warning developers about this limitation.Apply this diff to add a clarifying comment:
- // Special prefixes (/__vue-router/auto-routes, /@vite-plugin-layouts/, etc) + // Special prefixes (/__vue-router/auto-routes, /@vite-plugin-layouts/, etc) + // Note: API routes starting with /__ or /@ require a corresponding file in project root !/^\/(?:__|@)/.test(req.url!)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/build/vite/dev.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/build/vite/dev.ts (1)
src/runtime/internal/vite/dev-worker.mjs (2)
server(175-188)req(141-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-rolldown (windows-latest)
- GitHub Check: tests-rollup (ubuntu-latest)
- GitHub Check: tests-rollup (windows-latest)
🔇 Additional comments (3)
src/build/vite/dev.ts (3)
245-246: LGTM! Clear documentation of middleware ordering.The comment explains why this middleware runs early and provides a reference to the upstream Vite PR. This helps future maintainers understand the design decision.
249-249: Good fix! Vary header now set unconditionally.This correctly addresses the previous review comment by ensuring caches know the response varies on the
sec-fetch-destheader, preventing incorrect cached responses.
251-252: LGTM! Clear fetch destination filtering.The condition and comment correctly describe the logic for allowing document-like requests while excluding resource requests (scripts, styles, etc.) that Vite should handle.
1 participant
(from pair programming with @atinux on chat-vue template)
Nitro vite plugin adds an early vite dev middleware to handle SSR/API requests before overlapping with vite's root-level static asset serving.
We skip the early handler for requests with an extension but it is not enough as some plugins such as unplugin-vue-router use
/__virtual prefix and vite-plugin-layouts uses/@prefix.This PR skips for these special patterns too.
Note: API routes still work with routes starting with
/__or/@only if there is a file named like/_testor@fooin root of project, it will be wrongly served.