Conversation
commit: |
|
|
/bigbonk review please |
There was a problem hiding this comment.
Review: Deploy Auth UX
All CI checks are green (Lint, Typecheck, Vitest, all 5 E2E projects, deploys, smoke tests). The code is well-structured and the test coverage is thorough.
What's Good
Auth planning is cleanly factored. The WranglerAuthPlan discriminated union (token | interactive | login | error) makes the auth decision tree explicit and easy to test. Each scenario maps to a clear code path in ensureWranglerAuth, and the planWranglerAuth function is fully pure — env, tty state, saved login, and account ID are all injected, making the tests deterministic without mocking globals.
Account ID resolution cascade is correct. resolveWranglerAccountId checks env var → wrangler config (with env-specific override) in the right order. The parseWranglerConfig changes to accept targetEnv are clean — env-specific blocks take precedence for account_id, KV namespaces, routes, and custom domains in both JSON and TOML, with top-level as fallback.
Cloudflare plugin injection at deploy time is the right call. getDeployBuildConfig loads the user's Vite config, checks if the Cloudflare plugin is present by plugin name (not just text matching), and injects it only when missing. This keeps vinext init host-neutral while making vinext deploy just work. The configNeedsCloudflarePlugin helper correctly handles nested plugin arrays via flattenPlugins.
Error messages are actionable. Both buildWranglerAuthHelpMessage and buildWranglerAccountIdHelpMessage include exact env var names, doc links for every step, and differentiate between local terminal and CI scenarios. The test assertions verify all the links are present.
README docs match the implementation. The prerequisites section now accurately describes the interactive login flow, env var auth, and account ID resolution order.
Minor Observations
-
hasSavedWranglerLoginshells out towrangler whoami— this adds a ~1-2s synchronous call before the build starts in interactive mode. Acceptable for deploy UX, just noting it's there. -
ensureWranglerAuthcallsprocess.exit(1)directly — this is consistent with how the rest of the deploy pipeline handles fatal errors (the "no app/ or pages/" check does the same). Fine for CLI code, though it does mean this path is untestable in-process. The planWranglerAuth tests cover the decision logic thoroughly enough. -
TOML env block parsing (
getTOMLEnvBlock) usesindexOf+search(/^\[/m)to find the next section header. This works for standard wrangler.toml files but would break on edge cases like[env.preview]appearing inside a multi-line string value. Not a real concern — wrangler.toml files don't contain that pattern in practice. -
runWranglerDeployreturnsnullin interactive mode (sincestdio: "inherit"means we can't capture output). The caller correctly handles this withif (url)— clean.
Looks good to merge.
|
Approved the PR. Here's my review summary: All 23 CI checks pass. Code is well-structured with good test coverage. Key strengths:
Minor observations (none blocking):
|
Fixes #312
Summary
vinext deploywrangler login, and fail early in CI with exact token/account guidanceCLOUDFLARE_ACCOUNT_IDfrom env or Wrangler config, including selected--envblocksorigin/mainbefore starting workBehavior By Scenario
vinext deploystays Cloudflare-specific; Nitro remains the multi-platform path.wrangleris missing,vinext deployinstalls it automatically.wrangler loginand then continues.CLOUDFLARE_API_TOKEN, vinext fails early with a message that points to Wrangler login docs for local use and Cloudflare token docs for CI use.CLOUDFLARE_API_TOKENbut no account ID, vinext fails early before build.CLOUDFLARE_ACCOUNT_ID, top-levelaccount_id, or env-specificaccount_idin the selected Wrangler env block.vite.config.*is missing@cloudflare/vite-plugin, vinext warns but still injects the plugin for the deploy build so the deploy can proceed.vinext initremains host-neutral; Cloudflare-specific behavior stays invinext deploy.Testing
npx vitest run tests/deploy.test.ts tests/init.test.tspnpm run typecheck