fix(vite): keep ?import module requests on vite in dev#4453
Conversation
An imported `.json` has no extension in `ASSET_EXT_RE`, so with `Sec-Fetch-Dest` absent the dev middleware handed `/x.json?import` to the SSR catch-all, which answered it with HTML. Vite only ever tags module graph fetches with `?import`, so treat that query as an authoritative asset signal in the fallback branch.
|
@n0liu is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe Vite dev middleware now recognizes ChangesVite import routing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dev.ts`:
- Around line 27-30: Update the Vite import detection using VITE_IMPORT_QUERY_RE
so it evaluates only the URL query portion, such as parsed url.search or
URLSearchParams, rather than the full URL pathname. Preserve detection of the
import parameter while ensuring paths like /page&import are not classified as
module requests, and add a regression case covering that pathname.
In `@test/vite/app.test.ts`:
- Around line 85-96: Strengthen the assertions in the test “does not let the SSR
catch-all swallow ?import module requests” to verify Vite handled the request:
assert the expected missing-module HTTP status and that the response is
non-HTML, or use an existing fixture and assert its transformed JavaScript
response instead of only checking status is not 200.
🪄 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: 9d2c934d-2423-4d73-ac8e-97de0a0780a3
📒 Files selected for processing (2)
src/build/vite/dev.tstest/vite/app.test.ts
| // Vite tags module graph fetches for files it has to serve as modules (e.g. an imported `.json`) | ||
| // with an `?import` query. Only the module graph emits it — a page navigation never does — so it | ||
| // stays authoritative for extensions deliberately left out of `ASSET_EXT_RE` (#4433). | ||
| const VITE_IMPORT_QUERY_RE = /[?&]import(?:[&=]|$)/; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict ?import detection to the query string.
The regex is applied to the entire URL, so a valid pathname such as /page&import can be misclassified as a Vite module request when Sec-Fetch-Dest is absent or empty. Parse url.search/URLSearchParams first, then check for the import parameter; add a regression case for this pathname.
🤖 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/dev.ts` around lines 27 - 30, Update the Vite import detection
using VITE_IMPORT_QUERY_RE so it evaluates only the URL query portion, such as
parsed url.search or URLSearchParams, rather than the full URL pathname.
Preserve detection of the import parameter while ensuring paths like
/page&import are not classified as module requests, and add a regression case
covering that pathname.
| // #4433: Vite marks module-graph fetches for files it has to serve as modules with an `?import` | ||
| // query. The marker only ever comes from the module graph, never from a page navigation, so such | ||
| // a request must reach Vite even when the extension is not a known asset type (`.json`) and | ||
| // `Sec-Fetch-Dest` is absent. | ||
| test("does not let the SSR catch-all swallow ?import module requests", async () => { | ||
| const res = await fetch(`${serverURL}/missing-module.json?import`, { | ||
| headers: { accept: "*/*" }, | ||
| redirect: "manual", | ||
| }); | ||
| expect(res.status).not.toBe(200); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the Vite response more specifically.
not.toBe(200) can pass for unrelated failures such as a 500 or redirect, so this does not prove the request bypassed Nitro’s SSR catch-all. Assert the expected missing-module status and non-HTML response, or use an existing fixture and verify Vite’s transformed JavaScript response.
🤖 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/vite/app.test.ts` around lines 85 - 96, Strengthen the assertions in the
test “does not let the SSR catch-all swallow ?import module requests” to verify
Vite handled the request: assert the expected missing-module HTTP status and
that the response is non-HTML, or use an existing fixture and assert its
transformed JavaScript response instead of only checking status is not 200.
1 participant
Fixes #4433
In dev, a module graph fetch for an imported
.json(/x.json?import) was answered by the SSR catch-all with HTML instead of the transformed module wheneverSec-Fetch-Destwas absent (plain-HTTP non-loopback origins):jsonis not inASSET_EXT_RE, so the extension fallback classified the request as a navigation.Rather than widening
ASSET_EXT_RE, this keys off the?importquery Vite puts on module graph fetches. That marker only ever comes from the module graph — a page navigation never carries it — so it stays authoritative for the extensions the regex deliberately leaves out, without divertingpublic/*.jsonor catch-all JSON routes to Vite the way widening the regex would.src/build/vite/dev.ts— treat?importas an asset signal in the fallback branch, the one reached whenSec-Fetch-Destis absent orempty.test/vite/app.test.ts— regression test next to the existing Accessing via a non-localhost URL breaks vite dev server #4234 cases. It fails onmain(the SSR catch-all answers 200) and passes with the fix.Verified locally:
vitest run test/vite/(40 passed, no regressions in the #4234/#4241/#4252/#4270 routing cases),pnpm lintandpnpm typecheckclean.