refactor(cli): spec-driven command framework (dev/build/start/init migrated)#1874
refactor(cli): spec-driven command framework (dev/build/start/init migrated)#1874james-elicx wants to merge 4 commits into
Conversation
Introduce a zero-dependency CLI command framework under src/cli/ where a single CommandSpec is the source of truth for both argument parsing and help text, eliminating the drift between the hand-rolled parser and the separate inline help templates. Migrates the `dev` command onto the framework as the first slice; all other commands keep using the existing parseArgs/printHelp path for now. - cli/types.ts: CommandSpec/ArgSpec definitions + InferValues typing - cli/parse.ts: node:util parseArgs wrapper with typed coercion and the value-validation guards ported from cli-args.ts (CliUsageError) - cli/help.ts: help rendered from the same spec (TTY-aware styling) - cli/command.ts: defineCommand + runCommand (handles --help, prints clean usage errors with a help hint) - cli/runtime.ts: extracted shared Vite helpers (loadVite, getViteVersion, buildViteConfig, applyViteConfigCompatibility) so commands no longer depend on cli.ts - cli/commands/dev.ts: dev command spec + run, with heavy runtime imports lazy-loaded so the module stays cheap to import and unit test - tests/cli-framework.test.ts: parse + help-generation coverage
commit: |
|
| turbopack: { | ||
| type: "boolean", | ||
| description: "Accepted for compatibility (no-op, Vite is always used)", | ||
| }, |
There was a problem hiding this comment.
can we add an option to hide a flag from the help menu so that we can hide this turbopack one
There was a problem hiding this comment.
Done in a5d3a52. Added a hidden?: boolean option to ArgSpec; --turbopack is now hidden: true (and I added a hidden --experimental-https no-op too). Hidden flags are still parsed/accepted but omitted from --help.
| * given an empty value, is followed by something that looks like another | ||
| * flag, or fails type coercion (e.g. a non-integer `--port`). | ||
| */ | ||
| export function parseCommand<A extends Record<string, ArgSpec>>( |
There was a problem hiding this comment.
can we error for unknown flags
There was a problem hiding this comment.
Done in a5d3a52. parseCommand now errors on undeclared flags by default (Unknown option "--foo". + a Run \vinext --help`hint, exit 1). For commands that must tolerate arbitrary pass-through flags there is a per-commandpassthroughUnknown?: booleanopt-in (off by default). To keepnext devdrop-in usage working under the stricter policy, I declared the legacy--turbopackand--experimental-httpsflags ondev` as hidden no-ops.
| // Lazy-load the heavy runtime helpers (which pull in the full vinext Vite | ||
| // plugin) so importing this command module — e.g. for unit-testing its | ||
| // spec and help output — stays cheap and side-effect-free. | ||
| const { applyViteConfigCompatibility, buildViteConfig, getViteVersion, loadVite } = | ||
| await import("../runtime.js"); | ||
| const { loadDotenv } = await import("../../config/dotenv.js"); | ||
| const { formatAlreadyRunningError, tryAcquireLockfile } = | ||
| await import("../../server/dev-lockfile.js"); |
There was a problem hiding this comment.
kinda pointless, we can hoist these up
There was a problem hiding this comment.
Done in a5d3a52 — hoisted back to static module-level imports.
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — spec-driven CLI framework (dev slice)
Reviewed the new cli/ framework and the dev migration against the legacy cli-args.ts for parity. The framework is clean, well-documented, and the parse/help single-source-of-truth design is solid. Tests are thorough (86 green incl. legacy). Validation messages match the old parser exactly (-p vs --port spelling, Number()-based coercion, missing/empty/looks-like-a-flag guards), including --port= empty inline values and trailing-token consumption.
Found no correctness regressions for the dev command itself. A few latent issues for the framework that's about to grow as more commands migrate:
1. boolean + multiple silently collapses to the default (latent)
parse.ts:161-164 — the boolean branch runs before the multiple branch and checks raw === true. If a future spec declares a boolean with multiple: true, node:util returns an array ([true, true]), so raw === true is false and the flag resolves to arg.default ?? false — i.e. a repeated boolean flag reads as not set. No current command hits this (dev's only boolean isn't multiple), but since this is foundation code for the stacked migration, either reject boolean + multiple in defineCommand/types or handle the array case (raw === true || (Array.isArray(raw) && raw.length > 0)).
2. boolean default: true can never be turned off (design gap)
parse.ts:162 — a boolean with default: true resolves to true whether absent or present, with no negation (--no-foo) path. The ArgSpec.default doc says "for boolean args the default is always false," and InferValues types booleans as always-present, but nothing enforces default !== true for booleans. Consider constraining the type so a boolean can't declare a truthy default, to avoid a future footgun.
3. Unknown-flag policy is global, not per-command
parse.ts:126 (strict: false) drops all unknown flags for every command. The PR notes this is deliberate for next drop-in friendliness, and a reviewer already flagged wanting errors for unknown flags. As the framework absorbs commands like deploy/init where a typo'd flag silently doing nothing is worse than a hard error, consider making leniency a per-spec opt-in (e.g. passthroughUnknown?: boolean) rather than a hardcoded global. Not blocking for the dev slice.
Nits
help.ts:25—int/positiveIntfall through to the<n>placeholder; fine, but avalueHintis worth setting on those flags when they land so help reads better than--foo <n>.- Parity is preserved for
dev, but note--experimental-https(parsed by the legacycli-args.ts) is now only an ignored passthrough flag under the newdevpath. That matches the PR's stated intent (it was undocumented before), just calling it out so it's a conscious drop rather than an accidental one.
None of these block the dev slice. Items 1–2 are worth fixing in this PR since they're in the framework core that the rest of the stack will build on.
|
Review posted to PR #1874. Summary of findings: Review summaryThe spec-driven CLI framework is clean and well-tested. I verified parity with the legacy I flagged 3 latent framework issues (since this is foundation code for the stacked migration) plus nits:
Nits: None block the dev slice; items 1–2 are worth fixing in this PR since they're in the core. |
Review feedback from #1874: - Error on unknown flags by default instead of silently ignoring them, with a per-command `passthroughUnknown` opt-in for cases that need leniency. - Add a `hidden` flag option; hide the `--turbopack` no-op on `dev` and add a hidden `--experimental-https` no-op so `next dev` flags don't trip the new unknown-flag error. - Model ArgSpec as a discriminated union so the compiler rejects boolean+multiple and a truthy boolean default (both latent footguns flagged in review). - Hoist dev's lazy dynamic imports back to static module-level imports. Command migrations: - Migrate `build`, `start`, and `init` onto the framework (spec-driven parsing and help). Move createBuildLogger/hasPagesDir/loadBuildEmptyOutDir into the build command module and drop their printHelp templates + legacy parseArgs use. Tests: 100 total (48 framework + 52 legacy) covering unknown-flag errors, passthrough, hidden flags, dev compat flags, and build/start/init parse+help.
Review addressed +
|
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
… `next build --turbopack` compat - types.ts: ArgType, BooleanArgSpec, ValueArgSpec, PositionalSpec, ExampleSpec, and CommandContext are only referenced within the module, so stop exporting them (knip failed CI on the unused exports). ArgSpec, CommandSpec, and InferValues remain the public surface. - build: declare --turbopack as a hidden no-op (parity with dev). The legacy parser accepted it on every command and `next build --turbopack` is a real Next.js flag, so erroring on it would break drop-in usage. - tests: cover the hidden build flag; refresh the stale header comment.
james-elicx
changed the title
CI fixed + bonk review verified at HEAD (fa86678)Check job failure
Bonk review (2026-06-09 14:16) — verified addressed at HEAD
Self-review
Verification
|
|
/bigbonk review for issues |
/bigbonk review — spec-driven CLI framework (dev/build/start/init)Re-reviewed at HEAD (
No correctness regressions. Two minor items, both in the framework core that the rest of the stack will build on: 1. Stale module docstring in
|
|
Review posted to PR #1874. SummaryI reviewed the spec-driven CLI framework PR. The framework is clean and the No correctness regressions. I flagged two minor framework-core items:
Neither blocks the merge. |
…ocstring - parse.ts: with strict: false, node:util stores `--verbose=true` as the string "true", which failed the `raw === true` check and silently resolved the flag to false — the opposite of what the user typed. Raise a CliUsageError instead, consistent with the strict-by-default policy. - parse.ts: the module docstring still described the pre-review "graceful pass-through of unknown flags" design; document the strict-by-default + passthroughUnknown opt-in policy instead.
Bonk round 2 addressed (0664f8c)
|
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
1 participant
Summary
Introduces a zero-dependency, spec-driven CLI command framework under
packages/vinext/src/cli/where a singleCommandSpecis the source of truth for both argument parsing and help text. This eliminates the long-standing drift between the hand-rolled parser (cli-args.ts), thenode:utilparser (parseDeployArgs), and the separate inlineprintHelptemplates.This is the first slice of a stacked refactor (per AGENTS.md "prefer small stacked PRs"). It builds the framework and migrates the
dev,build,start, andinitcommands end-to-end (devin the first commit as the proof, the other three after review feedback). The remaining commands (lint,check,typegen,deploy) keep using the existingparseArgs/printHelppath and are untouched.Why
dev's flags were parsed incli-args.tsbut documented in a separate template string inprintHelp("dev"). Adding a flag meant editing two disconnected places. The drift was already real (e.g.--experimental-httpsis parsed but documented nowhere; global flags only appear underbuild's help).What's in this slice
cli/types.tsCommandSpec/ArgSpec+InferValuestype inferencecli/parse.tsnode:utilparseArgs wrapper; typed coercion (port/int/positiveInt/string/bool) + the value-validation guards ported fromcli-args.ts(CliUsageError); unknown flags are a hard error by default with a per-commandpassthroughUnknownopt-incli/help.tshiddenflags omitted)cli/command.tsdefineCommand+runCommand(handles--help, prints clean usage errors with a help hint)cli/runtime.tsloadVite,getViteVersion,buildViteConfig,applyViteConfigCompatibility) so commands no longer depend oncli.tscli/commands/{dev,build,start,init}.tsrunbodiestests/cli-framework.test.tscli.tsnow routesdev/build/start/initthroughrunCommand(...); the old command bodies and their hand-written help templates are removed.Design notes
node:utilparseArgs (already used bydeploy). No new dependencies.Unknown option "--foo".+ a help hint, exit 1) instead of being silently ignored, per review feedback. Commands that must tolerate arbitrary flags can opt in viapassthroughUnknown. KnownnextCLI flags that vinext doesn't need (--turbopackon dev/build,--experimental-httpson dev) are declared as hidden no-ops so drop-in usage keeps working.cli-args.tsare preserved — missing values, empty values, "looks like another flag", strict integer/port/positive-int coercion — with messages that reference the exact form the user typed (-pvs--port).Verification
vp test run tests/cli-framework.test.ts tests/cli-args.test.ts→ 101 passed (legacycli-argssuite still green;cli-args.tsintentionally untouched this slice).vp check+knip→ format + lint + types + unused-exports clean.vp run vinext#build→ builds successfully.dist/cli.js:vinext dev --help/-h,vinext build --help,vinext start --help,vinext init --help→ generated help (description + Examples; hidden compat flags omitted).vinext --help(root) and legacy-path commands (lint,check,typegen,deploy) → unchanged.dev -p abc,dev --port(missing),dev --port 70000(range),dev --port --hostname x(looks-like-flag),build --bogus(unknown option) → clean message +Run \vinext --help`` hint + exit 1.vinext dev -p 31847intests/fixtures/app-basic→ boots, prints banner + Local URL.Next steps (follow-up PRs)
lint,check,typegen,deploy) onto the framework, then move root help/version/dispatch into arunCli(registry)and deletecli-args.ts+ theprintHelptemplates.@clack/prompts) toinit/deployvia a TTY-guardedcli/prompt.tswith--yes/non-interactive fallbacks. (Adding that dependency will require a maintainer to run the install, since the lockfile is frozen.)