fix: warn when existing vite.config.ts is missing cloudflare plugin on deploy#315
Conversation
…n deploy Closes #312. - Add `viteConfigHasCloudflarePlugin()` to deploy.ts: when `vinext deploy` finds an existing vite.config.ts that doesn't import @cloudflare/vite-plugin, it now prints an actionable warning with the exact config fix rather than silently proceeding to a broken build. - Add README prerequisites section to the Cloudflare Workers deploy docs: covers wrangler login vs CLOUDFLARE_API_TOKEN (with required permissions) and how to supply account_id. - Add 7 unit tests for `viteConfigHasCloudflarePlugin()`.
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
Good change — this addresses a real pain point where vinext init → vinext deploy silently fails with a cryptic error. The detection function, warning placement, tests, and README additions are all well done.
One bug to fix before merging:
Bug: Pages Router warning shows invalid syntax
In the warning message at deploy.ts:1147-1149, the Pages Router branch produces cloudflare(()) instead of cloudflare():
cloudflare(${info.isAppRouter ? `{
viteEnvironment: { name: "rsc", childEnvironments: ["ssr"] },
}` : "()"}),When info.isAppRouter is false, the template interpolates () inside the existing cloudflare(...), producing:
cloudflare(()),
That's a JS syntax error. Fix by changing "()" to "":
cloudflare(${info.isAppRouter ? `{
viteEnvironment: { name: "rsc", childEnvironments: ["ssr"] },
}` : ""}),Or restructure the ternary to include cloudflare(...) in both branches.
Everything else looks good
viteConfigHasCloudflarePlugin()— clean heuristic, checks all three config variants, safe default on read errors- The guard condition
info.hasViteConfig && !viteConfigHasCloudflarePlugin(root)is correct — only fires when a user-supplied config exists but is missing the plugin - Tests cover both positive and negative cases across
.ts/.js/.mjs, including the exactvinext init-generated config that triggers issue #312 - README prerequisites section is clear and covers both auth methods and account ID configuration
|
Review posted. Here's a summary: Reviewed PR #315 — The PR is solid overall. It addresses a real UX problem where One bug found: The warning message for Pages Router projects renders Everything else — the detection function, test coverage (7 new tests), guard condition placement in the deploy flow, and README prerequisites docs — is well done. Requested changes for the syntax bug fix. |
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-review
The cloudflare(()) bug from the previous review is fixed — the ternary else branch now correctly uses "" (empty string), producing cloudflare() for Pages Router projects.
Everything else remains solid:
- Detection function — simple string-includes heuristic is the right call here. Covers
.ts/.js/.mjsvariants, fails safe on read errors (returns true to avoid false warnings), returns false when no config exists at all - Guard condition —
info.hasViteConfig && !viteConfigHasCloudflarePlugin(root)correctly limits the warning to cases where a user-supplied config exists but is missing the plugin. Won't fire when vinext auto-generates the config (since that path writes the plugin) - Warning is non-blocking — uses
console.warnand continues, which is the right severity for a heuristic check. The user can still proceed if they have an unusual setup - Test coverage — 7 tests cover positive/negative cases across all config file variants plus the exact
vinext init-generated config that triggers #312 - README prerequisites — clear, covers both auth methods and account ID
LGTM.
|
Re-review posted and approved. The |
1 participant
Summary
Fixes #312.
Three blockers were reported in the
npm create next-app→npx vinext init→ deploy flow:CLOUDFLARE_API_TOKENpermissions not documentedaccount_idmissing from generatedwrangler.jsonc, with no docs on how to supply itcould not resolve virtual:vinext-rsc-entrybecausevinext initgenerates a minimal local-devvite.config.tswithout@cloudflare/vite-plugin, andvinext deploysilently skips config generation when the file already existsChanges
deploy.ts— addviteConfigHasCloudflarePlugin(): whenvinext deploydetects an existingvite.config.tsthat doesn't import@cloudflare/vite-plugin, it now emits a clear warning with the exact config fix instead of silently proceeding to a broken buildREADME.md— add a "Prerequisites" section to the Cloudflare Workers deployment docs covering:wrangler loginvsCLOUDFLARE_API_TOKEN(with correct "Edit Cloudflare Workers" template permissions), and how to supplyaccount_idtests/deploy.test.ts— 7 new unit tests forviteConfigHasCloudflarePlugin()Testing