From f88ed06f6a8f592b33e8d31bdfb66283d5c2eb7e Mon Sep 17 00:00:00 2001 From: MK Date: Fri, 21 Aug 2026 13:11:13 +0800 Subject: [PATCH 01/19] refactor(cli): parse staged args with clap --- .../snapshots/command_staged_concurrent.md | 16 +- .../snapshots/command_staged_help.md | 6 +- packages/cli/binding/index.cjs | 1 + packages/cli/binding/index.d.cts | 30 + .../cli/binding/src/js_command_args/mod.rs | 2 + .../cli/binding/src/js_command_args/parse.rs | 50 ++ .../cli/binding/src/js_command_args/staged.rs | 298 +++++++++ packages/cli/binding/src/lib.rs | 2 + .../cli/src/staged/__tests__/args.spec.ts | 76 ++- packages/cli/src/staged/args.ts | 107 --- packages/cli/src/staged/bin.ts | 59 +- rfcs/napi-clap-cli-args.md | 616 ++++++++++++++++++ 12 files changed, 1101 insertions(+), 162 deletions(-) create mode 100644 packages/cli/binding/src/js_command_args/mod.rs create mode 100644 packages/cli/binding/src/js_command_args/parse.rs create mode 100644 packages/cli/binding/src/js_command_args/staged.rs delete mode 100644 packages/cli/src/staged/args.ts create mode 100644 rfcs/napi-clap-cli-args.md diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_concurrent/snapshots/command_staged_concurrent.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_concurrent/snapshots/command_staged_concurrent.md index 598fc112bc..f10d48e736 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_concurrent/snapshots/command_staged_concurrent.md +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_concurrent/snapshots/command_staged_concurrent.md @@ -39,7 +39,9 @@ zero concurrency should fail before starting lint-staged ``` VITE+ - The Unified Toolchain for the Web -error: Option "--concurrent" must be true, false, or a number greater than 0. +error: invalid value '0' for '--concurrent []': must be true, false, or an integer from 1 to 4294967295 + +For more information, try '--help'. ``` ## `vp staged --no-cwd` @@ -51,7 +53,11 @@ negated string options should report a CLI error ``` VITE+ - The Unified Toolchain for the Web -error: Option "--no-cwd" is not supported. Use "--cwd ". +error: unexpected argument '--no-cwd' found + +Usage: vp staged [OPTIONS] + +For more information, try '--help'. ``` ## `vp staged --no-diff` @@ -63,5 +69,9 @@ negated diff should report a CLI error ``` VITE+ - The Unified Toolchain for the Web -error: Option "--no-diff" is not supported. Use "--diff ". +error: unexpected argument '--no-diff' found + +Usage: vp staged [OPTIONS] + +For more information, try '--help'. ``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_help/snapshots/command_staged_help.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_help/snapshots/command_staged_help.md index 80d0f60290..a8d7194027 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_help/snapshots/command_staged_help.md +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_staged_help/snapshots/command_staged_help.md @@ -11,7 +11,7 @@ Run linters on staged files using staged config from vite.config.ts. Options: --allow-empty Allow empty commits when tasks revert all staged changes - -p, --concurrent Number of tasks to run concurrently, or false for serial + -p, --concurrent [number|boolean] Number of tasks to run concurrently, or false for serial --continue-on-error Run all tasks to completion even if one fails --cwd Working directory to run all tasks in -d, --debug Enable debug output @@ -41,7 +41,7 @@ Run linters on staged files using staged config from vite.config.ts. Options: --allow-empty Allow empty commits when tasks revert all staged changes - -p, --concurrent Number of tasks to run concurrently, or false for serial + -p, --concurrent [number|boolean] Number of tasks to run concurrently, or false for serial --continue-on-error Run all tasks to completion even if one fails --cwd Working directory to run all tasks in -d, --debug Enable debug output @@ -71,7 +71,7 @@ Run linters on staged files using staged config from vite.config.ts. Options: --allow-empty Allow empty commits when tasks revert all staged changes - -p, --concurrent Number of tasks to run concurrently, or false for serial + -p, --concurrent [number|boolean] Number of tasks to run concurrently, or false for serial --continue-on-error Run all tasks to completion even if one fails --cwd Working directory to run all tasks in -d, --debug Enable debug output diff --git a/packages/cli/binding/index.cjs b/packages/cli/binding/index.cjs index 31579e337a..9dc2053244 100644 --- a/packages/cli/binding/index.cjs +++ b/packages/cli/binding/index.cjs @@ -969,6 +969,7 @@ module.exports.getVpDirs = nativeBinding.getVpDirs; module.exports.hasConfigKey = nativeBinding.hasConfigKey; module.exports.mergeJsonConfig = nativeBinding.mergeJsonConfig; module.exports.mergeTsdownConfig = nativeBinding.mergeTsdownConfig; +module.exports.parseStagedArgs = nativeBinding.parseStagedArgs; module.exports.rewriteEslint = nativeBinding.rewriteEslint; module.exports.rewriteImportsInDirectory = nativeBinding.rewriteImportsInDirectory; module.exports.rewritePrettier = nativeBinding.rewritePrettier; diff --git a/packages/cli/binding/index.d.cts b/packages/cli/binding/index.d.cts index af6b9c8524..c4302090ff 100644 --- a/packages/cli/binding/index.d.cts +++ b/packages/cli/binding/index.d.cts @@ -3463,6 +3463,11 @@ export interface CliOptions { resolveUniversalViteConfig: (err: Error | null, arg: string) => Promise; } +export interface CliParseError { + kind: string; + message: string; +} + /** * Detect the workspace root and package manager type and version * @@ -3651,6 +3656,13 @@ export declare function mergeTsdownConfig( tsdownConfigPath: string, ): MergeJsonConfigResult; +export declare function parseStagedArgs(argv: Array): ParseStagedArgsOutcome; + +export type ParseStagedArgsOutcome = + | { status: 'ok'; value: StagedArgs } + | { status: 'help' } + | { status: 'error'; error: CliParseError }; + /** Access modes for a path. */ export interface PathAccess { /** Whether the path was read */ @@ -3835,6 +3847,24 @@ export interface RunCommandResult { */ export declare function shouldPrintVitePlusHeader(): boolean; +export interface StagedArgs { + allowEmpty?: boolean; + concurrent?: boolean | number; + continueOnError?: boolean; + cwd?: string; + debug?: boolean; + diff?: string; + diffFilter?: string; + failOnChanges?: boolean; + hidePartiallyStaged?: boolean; + hideUnstaged?: boolean; + quiet?: boolean; + relative?: boolean; + revert?: boolean; + stash?: boolean; + verbose?: boolean; +} + /** * Set the value of a top-level config key in a vite config file (upsert) * diff --git a/packages/cli/binding/src/js_command_args/mod.rs b/packages/cli/binding/src/js_command_args/mod.rs new file mode 100644 index 0000000000..2e8ebb68bb --- /dev/null +++ b/packages/cli/binding/src/js_command_args/mod.rs @@ -0,0 +1,2 @@ +mod parse; +mod staged; diff --git a/packages/cli/binding/src/js_command_args/parse.rs b/packages/cli/binding/src/js_command_args/parse.rs new file mode 100644 index 0000000000..a17a0186c0 --- /dev/null +++ b/packages/cli/binding/src/js_command_args/parse.rs @@ -0,0 +1,50 @@ +use std::iter; + +use clap::{Args, Command, FromArgMatches, error::ErrorKind}; +use napi_derive::napi; + +#[napi(object, object_from_js = false)] +pub struct CliParseError { + pub kind: String, + pub message: String, +} + +pub(super) enum ParseResult { + Ok(T), + Help, + Error(CliParseError), +} + +pub(super) fn parse_args(bin_name: &'static str, argv: Vec) -> ParseResult +where + T: Args + FromArgMatches, +{ + let command = T::augment_args(Command::new(bin_name)); + let parsed = command + .try_get_matches_from(iter::once(bin_name.to_owned()).chain(argv)) + .and_then(|mut matches| T::from_arg_matches_mut(&mut matches)); + + match parsed { + Ok(value) => ParseResult::Ok(value), + Err(error) if error.kind() == ErrorKind::DisplayHelp => ParseResult::Help, + Err(error) => ParseResult::Error(CliParseError { + kind: error_kind_name(error.kind()).to_owned(), + message: error.to_string().trim_end().to_owned(), + }), + } +} + +fn error_kind_name(kind: ErrorKind) -> &'static str { + match kind { + ErrorKind::InvalidValue + | ErrorKind::NoEquals + | ErrorKind::ValueValidation + | ErrorKind::TooManyValues + | ErrorKind::TooFewValues + | ErrorKind::WrongNumberOfValues => "invalid-value", + ErrorKind::UnknownArgument | ErrorKind::InvalidSubcommand => "unknown-argument", + ErrorKind::ArgumentConflict => "argument-conflict", + ErrorKind::MissingRequiredArgument | ErrorKind::MissingSubcommand => "missing-argument", + _ => "invalid-arguments", + } +} diff --git a/packages/cli/binding/src/js_command_args/staged.rs b/packages/cli/binding/src/js_command_args/staged.rs new file mode 100644 index 0000000000..8759a396a1 --- /dev/null +++ b/packages/cli/binding/src/js_command_args/staged.rs @@ -0,0 +1,298 @@ +use std::{num::NonZeroU32, str::FromStr}; + +use clap::{ArgAction, Args}; +use napi::bindgen_prelude::Either; +use napi_derive::napi; + +use super::parse::{CliParseError, ParseResult, parse_args}; + +const CONCURRENT_VALUE_ERROR: &str = "must be true, false, or an integer from 1 to 4294967295"; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Concurrent { + Enabled, + Disabled, + Limit(NonZeroU32), +} + +impl FromStr for Concurrent { + type Err = &'static str; + + fn from_str(value: &str) -> Result { + match value { + "true" => Ok(Self::Enabled), + "false" => Ok(Self::Disabled), + value => { + value.parse::().map(Self::Limit).map_err(|_| CONCURRENT_VALUE_ERROR) + } + } + } +} + +#[derive(Debug, Args)] +struct StagedCliArgs { + #[arg(long, action = ArgAction::SetTrue)] + allow_empty: bool, + + #[arg( + short = 'p', + long, + value_name = "number|boolean", + num_args = 0..=1, + default_missing_value = "true", + allow_negative_numbers = true, + overrides_with = "no_concurrent" + )] + concurrent: Option, + + #[arg(long = "no-concurrent", overrides_with = "concurrent")] + no_concurrent: bool, + + #[arg(long, action = ArgAction::SetTrue)] + continue_on_error: bool, + + #[arg(long, value_name = "path")] + cwd: Option, + + #[arg(short = 'd', long, action = ArgAction::SetTrue)] + debug: bool, + + #[arg(long, value_name = "string")] + diff: Option, + + #[arg(long, value_name = "string")] + diff_filter: Option, + + #[arg(long, action = ArgAction::SetTrue)] + fail_on_changes: bool, + + #[arg(long, action = ArgAction::SetTrue)] + hide_partially_staged: bool, + + #[arg(long, action = ArgAction::SetTrue)] + hide_unstaged: bool, + + #[arg(long = "no-stash")] + no_stash: bool, + + #[arg(short = 'q', long, action = ArgAction::SetTrue)] + quiet: bool, + + #[arg(short = 'r', long, action = ArgAction::SetTrue)] + relative: bool, + + #[arg(long, action = ArgAction::SetTrue)] + revert: bool, + + #[arg(short = 'v', long, action = ArgAction::SetTrue)] + verbose: bool, +} + +#[napi(object, object_from_js = false)] +pub struct StagedArgs { + pub allow_empty: Option, + pub concurrent: Option>, + pub continue_on_error: Option, + pub cwd: Option, + pub debug: Option, + pub diff: Option, + pub diff_filter: Option, + pub fail_on_changes: Option, + pub hide_partially_staged: Option, + pub hide_unstaged: Option, + pub quiet: Option, + pub relative: Option, + pub revert: Option, + pub stash: Option, + pub verbose: Option, +} + +impl From for StagedArgs { + fn from(value: StagedCliArgs) -> Self { + let concurrent = if value.no_concurrent { + Some(Either::A(false)) + } else { + value.concurrent.map(|concurrent| match concurrent { + Concurrent::Enabled => Either::A(true), + Concurrent::Disabled => Either::A(false), + Concurrent::Limit(limit) => Either::B(limit.get()), + }) + }; + + Self { + allow_empty: value.allow_empty.then_some(true), + concurrent, + continue_on_error: value.continue_on_error.then_some(true), + cwd: value.cwd, + debug: value.debug.then_some(true), + diff: value.diff, + diff_filter: value.diff_filter, + fail_on_changes: value.fail_on_changes.then_some(true), + hide_partially_staged: value.hide_partially_staged.then_some(true), + hide_unstaged: value.hide_unstaged.then_some(true), + quiet: value.quiet.then_some(true), + relative: value.relative.then_some(true), + revert: value.revert.then_some(true), + stash: value.no_stash.then_some(false), + verbose: value.verbose.then_some(true), + } + } +} + +#[napi(discriminant = "status", discriminant_case = "camelCase", object_from_js = false)] +pub enum ParseStagedArgsOutcome { + Ok { value: StagedArgs }, + Help, + Error { error: CliParseError }, +} + +#[napi] +pub fn parse_staged_args(argv: Vec) -> ParseStagedArgsOutcome { + match parse_args::("vp staged", argv) { + ParseResult::Ok(value) => ParseStagedArgsOutcome::Ok { value: value.into() }, + ParseResult::Help => ParseStagedArgsOutcome::Help, + ParseResult::Error(error) => ParseStagedArgsOutcome::Error { error }, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(argv: &[&str]) -> ParseResult { + parse_args("vp staged", argv.iter().map(|value| (*value).to_owned()).collect()) + } + + fn parsed(argv: &[&str]) -> StagedCliArgs { + match parse(argv) { + ParseResult::Ok(value) => value, + ParseResult::Help => panic!("expected parsed arguments, got help"), + ParseResult::Error(error) => panic!("expected parsed arguments: {}", error.message), + } + } + + fn parse_error(argv: &[&str]) -> CliParseError { + match parse(argv) { + ParseResult::Error(error) => error, + ParseResult::Ok(_) => panic!("expected an argument error"), + ParseResult::Help => panic!("expected an argument error, got help"), + } + } + + #[test] + fn parses_concurrent_forms() { + let cases = [ + (&["--concurrent"][..], Concurrent::Enabled), + (&["--concurrent", "true"][..], Concurrent::Enabled), + (&["--concurrent=false"][..], Concurrent::Disabled), + (&["--concurrent=1"][..], Concurrent::Limit(NonZeroU32::MIN)), + (&["-p", "2"][..], Concurrent::Limit(NonZeroU32::new(2).expect("2 is non-zero"))), + ]; + + for (argv, expected) in cases { + assert_eq!(parsed(argv).concurrent, Some(expected)); + } + + let args = parsed(&["--no-concurrent"]); + assert!(args.no_concurrent); + assert_eq!(args.concurrent, None); + } + + #[test] + fn last_concurrent_form_wins() { + let args = parsed(&["--concurrent=2", "--no-concurrent"]); + assert!(args.no_concurrent); + assert_eq!(args.concurrent, None); + + let args = parsed(&["--no-concurrent", "--concurrent=2"]); + assert!(!args.no_concurrent); + assert_eq!( + args.concurrent, + Some(Concurrent::Limit(NonZeroU32::new(2).expect("2 is non-zero"))) + ); + } + + #[test] + fn rejects_invalid_concurrent_values() { + for value in ["0", "-1", "1.5", "NaN", "4294967296"] { + let error = parse_error(&["--concurrent", value]); + assert_eq!(error.kind, "invalid-value"); + assert!(error.message.contains(CONCURRENT_VALUE_ERROR)); + } + } + + #[test] + fn parses_supported_options() { + let args = parsed(&[ + "--allow-empty", + "--continue-on-error", + "--cwd", + "packages/app", + "--debug", + "--diff=main...HEAD", + "--diff-filter", + "ACMR", + "--fail-on-changes", + "--hide-partially-staged", + "--hide-unstaged", + "--no-stash", + "--quiet", + "--relative", + "--revert", + "--verbose", + ]); + + assert!(args.allow_empty); + assert!(args.continue_on_error); + assert_eq!(args.cwd.as_deref(), Some("packages/app")); + assert!(args.debug); + assert_eq!(args.diff.as_deref(), Some("main...HEAD")); + assert_eq!(args.diff_filter.as_deref(), Some("ACMR")); + assert!(args.fail_on_changes); + assert!(args.hide_partially_staged); + assert!(args.hide_unstaged); + assert!(args.no_stash); + assert!(args.quiet); + assert!(args.relative); + assert!(args.revert); + assert!(args.verbose); + } + + #[test] + fn returns_help_without_printing() { + assert!(matches!(parse(&["--help"]), ParseResult::Help)); + assert!(matches!(parse(&["-h"]), ParseResult::Help)); + assert!(matches!(parse(&["--help", "--unknown"]), ParseResult::Help)); + } + + #[test] + fn rejects_missing_and_repeated_values() { + assert_eq!(parse_error(&["--cwd"]).kind, "invalid-value"); + assert_eq!(parse_error(&["--cwd", "one", "--cwd", "two"]).kind, "argument-conflict"); + assert_eq!(parse_error(&["--debug", "--debug"]).kind, "argument-conflict"); + } + + #[test] + fn rejects_unsupported_and_positional_arguments() { + for argv in [ + &["--no-cwd"][..], + &["--no-diff"][..], + &["--stash"][..], + &["--no-debug"][..], + &["unexpected"][..], + ] { + let error = parse_error(argv); + assert_eq!(error.kind, "unknown-argument"); + } + } + + #[test] + fn maps_explicit_values_for_javascript() { + let args = StagedArgs::from(parsed(&["--concurrent=3", "--no-stash", "--debug"])); + + assert!(matches!(args.concurrent, Some(Either::B(3)))); + assert_eq!(args.stash, Some(false)); + assert_eq!(args.debug, Some(true)); + assert_eq!(args.quiet, None); + } +} diff --git a/packages/cli/binding/src/lib.rs b/packages/cli/binding/src/lib.rs index f860d74d04..0c8f00ff66 100644 --- a/packages/cli/binding/src/lib.rs +++ b/packages/cli/binding/src/lib.rs @@ -22,6 +22,8 @@ mod exec; // These modules export NAPI functions only called from JavaScript at runtime. // allow(dead_code) suppresses warnings in the test target which doesn't link NAPI. #[allow(dead_code)] +mod js_command_args; +#[allow(dead_code)] mod migration; #[allow(dead_code)] mod package_manager; diff --git a/packages/cli/src/staged/__tests__/args.spec.ts b/packages/cli/src/staged/__tests__/args.spec.ts index e206be66cd..a60187a360 100644 --- a/packages/cli/src/staged/__tests__/args.spec.ts +++ b/packages/cli/src/staged/__tests__/args.spec.ts @@ -1,6 +1,24 @@ import { describe, expect, it } from 'vitest'; -import { normalizeStagedArgs, parseStagedArgs } from '../args.js'; +import { parseStagedArgs } from '../../../binding/index.js'; + +function expectParsed(argv: string[]) { + const outcome = parseStagedArgs(argv); + expect(outcome.status).toBe('ok'); + if (outcome.status !== 'ok') { + throw new Error(`Expected parsed arguments, got ${outcome.status}`); + } + return outcome.value; +} + +function expectParseError(argv: string[]) { + const outcome = parseStagedArgs(argv); + expect(outcome.status).toBe('error'); + if (outcome.status !== 'error') { + throw new Error(`Expected an argument error, got ${outcome.status}`); + } + return outcome.error; +} describe('staged arguments', () => { it.each([ @@ -10,41 +28,63 @@ describe('staged arguments', () => { { argv: ['--concurrent'], expected: true }, { argv: ['--concurrent=1'], expected: 1 }, { argv: ['-p', '2'], expected: 2 }, - ])('normalizes $argv to concurrent=$expected', ({ argv, expected }) => { - expect(normalizeStagedArgs(parseStagedArgs(argv)).concurrent).toBe(expected); + ])('parses $argv as concurrent=$expected', ({ argv, expected }) => { + expect(expectParsed(argv).concurrent).toBe(expected); }); it.each([ { argv: ['--concurrent=0'] }, { argv: ['-p', '0'] }, { argv: ['--concurrent=-1'] }, + { argv: ['--concurrent=1.5'] }, { argv: ['--concurrent=NaN'] }, + { argv: ['--concurrent=4294967296'] }, ])('rejects invalid concurrency $argv', ({ argv }) => { - expect(() => normalizeStagedArgs(parseStagedArgs(argv))).toThrow( - 'Option "--concurrent" must be true, false, or a number greater than 0.', - ); + const error = expectParseError(argv); + expect(error.kind).toBe('invalid-value'); + expect(error.message).toContain('must be true, false, or an integer from 1 to 4294967295'); }); - it.each([ - { argv: ['--no-cwd'], option: 'cwd', value: 'path' }, - { argv: ['--no-diff'], option: 'diff', value: 'string' }, - { argv: ['--no-diff-filter'], option: 'diff-filter', value: 'string' }, - ])('rejects the negated string option --no-$option', ({ argv, option, value }) => { - expect(() => normalizeStagedArgs(parseStagedArgs(argv))).toThrow( - `Option "--no-${option}" is not supported. Use "--${option} <${value}>".`, - ); + it.each([['--no-cwd'], ['--no-diff'], ['--no-diff-filter'], ['--stash'], ['--no-debug']])( + 'rejects unsupported option %s', + (option) => { + const error = expectParseError([option]); + expect(error.kind).toBe('unknown-argument'); + expect(error.message).toContain(`unexpected argument '${option}'`); + }, + ); + + it('rejects positional arguments', () => { + const error = expectParseError(['unexpected']); + expect(error.kind).toBe('unknown-argument'); + }); + + it('rejects missing and repeated values', () => { + expect(expectParseError(['--cwd']).kind).toBe('invalid-value'); + expect(expectParseError(['--cwd', 'one', '--cwd', 'two']).kind).toBe('argument-conflict'); + expect(expectParseError(['--debug', '--debug']).kind).toBe('argument-conflict'); }); it('preserves valid string options', () => { expect( - normalizeStagedArgs( - parseStagedArgs(['--cwd', 'packages/app', '--diff=main...HEAD', '--diff-filter', 'ACMR']), - ), + expectParsed(['--cwd', 'packages/app', '--diff=main...HEAD', '--diff-filter', 'ACMR']), ).toEqual({ - concurrent: undefined, cwd: 'packages/app', diff: 'main...HEAD', diffFilter: 'ACMR', }); }); + + it('maps explicit boolean options without adding absent defaults', () => { + expect(expectParsed(['--allow-empty', '--debug', '--no-stash'])).toEqual({ + allowEmpty: true, + debug: true, + stash: false, + }); + }); + + it('returns help as data', () => { + expect(parseStagedArgs(['--help'])).toEqual({ status: 'help' }); + expect(parseStagedArgs(['--help', '--unknown'])).toEqual({ status: 'help' }); + }); }); diff --git a/packages/cli/src/staged/args.ts b/packages/cli/src/staged/args.ts deleted file mode 100644 index 95ccc00289..0000000000 --- a/packages/cli/src/staged/args.ts +++ /dev/null @@ -1,107 +0,0 @@ -import type { Options } from 'lint-staged'; -import mri from 'mri'; - -export interface StagedArgs { - help?: boolean; - concurrent?: unknown; - cwd?: unknown; - diff?: unknown; - 'diff-filter'?: unknown; - 'allow-empty'?: boolean; - debug?: boolean; - 'continue-on-error'?: boolean; - 'fail-on-changes'?: boolean; - 'hide-partially-staged'?: boolean; - 'hide-unstaged'?: boolean; - quiet?: boolean; - relative?: boolean; - revert?: boolean; - stash?: boolean; - verbose?: boolean; -} - -export interface NormalizedStagedArgs { - concurrent?: Options['concurrent']; - cwd?: string; - diff?: string; - diffFilter?: string; -} - -export function parseStagedArgs(argv: string[]): StagedArgs { - return mri(argv, { - alias: { - h: 'help', - p: 'concurrent', - d: 'debug', - q: 'quiet', - r: 'relative', - v: 'verbose', - }, - boolean: [ - 'help', - 'allow-empty', - 'debug', - 'continue-on-error', - 'fail-on-changes', - 'hide-partially-staged', - 'hide-unstaged', - 'quiet', - 'relative', - 'revert', - 'stash', - 'verbose', - ], - string: ['concurrent', 'cwd', 'diff', 'diff-filter'], - }); -} - -export function normalizeStagedArgs(args: StagedArgs): NormalizedStagedArgs { - return { - concurrent: normalizeConcurrent(args.concurrent), - cwd: normalizeStringOption(args.cwd, 'cwd', 'path'), - diff: normalizeStringOption(args.diff, 'diff', 'string'), - diffFilter: normalizeStringOption(args['diff-filter'], 'diff-filter', 'string'), - }; -} - -function normalizeConcurrent(value: unknown): Options['concurrent'] | undefined { - if (value == null) { - return undefined; - } - if (value === true || value === false) { - return value; - } - if (value === '') { - return true; - } - if (value === 'true') { - return true; - } - if (value === 'false') { - return false; - } - - const number = Number(value); - if (Number.isFinite(number) && number > 0) { - return number; - } - - throw new Error('Option "--concurrent" must be true, false, or a number greater than 0.'); -} - -function normalizeStringOption( - value: unknown, - option: string, - valueName: string, -): string | undefined { - if (value == null) { - return undefined; - } - if (value === false) { - throw new Error(`Option "--no-${option}" is not supported. Use "--${option} <${valueName}>".`); - } - if (typeof value !== 'string' || value === '') { - throw new Error(`Option "--${option}" requires a value.`); - } - return value; -} diff --git a/packages/cli/src/staged/bin.ts b/packages/cli/src/staged/bin.ts index 1f8264a841..4b2ae6bee3 100644 --- a/packages/cli/src/staged/bin.ts +++ b/packages/cli/src/staged/bin.ts @@ -12,14 +12,14 @@ import lintStaged from 'lint-staged'; import type { Options } from 'lint-staged'; +import { parseStagedArgs } from '../../binding/index.js'; import { resolveViteConfig } from '../resolve-vite-config.ts'; import { renderCliDoc } from '../utils/help.ts'; import { errorMsg, log, printHeader } from '../utils/terminal.ts'; -import { normalizeStagedArgs, parseStagedArgs } from './args.ts'; -const args = parseStagedArgs(process.argv.slice(3)); +const parsedArgs = parseStagedArgs(process.argv.slice(3)); -if (args.help) { +if (parsedArgs.status === 'help') { const helpMessage = renderCliDoc({ usage: 'vp staged [options]', summary: 'Run linters on staged files using staged config from vite.config.ts.', @@ -33,7 +33,7 @@ if (args.help) { description: 'Allow empty commits when tasks revert all staged changes', }, { - label: '-p, --concurrent ', + label: '-p, --concurrent [number|boolean]', description: 'Number of tasks to run concurrently, or false for serial', }, { @@ -74,36 +74,33 @@ if (args.help) { }); printHeader(); log(helpMessage); +} else if (parsedArgs.status === 'error') { + printHeader(); + errorMsg(parsedArgs.error.message.replace(/^error:\s*/, '')); + process.exit(1); } else { - let normalizedArgs; - try { - normalizedArgs = normalizeStagedArgs(args); - } catch (err) { - printHeader(); - errorMsg(err instanceof Error ? err.message : String(err)); - process.exit(1); - } + const args = parsedArgs.value; const options: Options = {}; // Boolean flags — only include if explicitly set - if (args['allow-empty'] != null) { - options.allowEmpty = args['allow-empty']; + if (args.allowEmpty != null) { + options.allowEmpty = args.allowEmpty; } if (args.debug != null) { options.debug = args.debug; } - if (args['continue-on-error'] != null) { - options.continueOnError = args['continue-on-error']; + if (args.continueOnError != null) { + options.continueOnError = args.continueOnError; } - if (args['fail-on-changes'] != null) { - options.failOnChanges = args['fail-on-changes']; + if (args.failOnChanges != null) { + options.failOnChanges = args.failOnChanges; } - if (args['hide-partially-staged'] != null) { - options.hidePartiallyStaged = args['hide-partially-staged']; + if (args.hidePartiallyStaged != null) { + options.hidePartiallyStaged = args.hidePartiallyStaged; } - if (args['hide-unstaged'] != null) { - options.hideUnstaged = args['hide-unstaged']; + if (args.hideUnstaged != null) { + options.hideUnstaged = args.hideUnstaged; } if (args.quiet != null) { options.quiet = args.quiet; @@ -124,7 +121,7 @@ if (args.help) { // Read "staged" from vite.config.ts and pass it as an inline config object to lint-staged. let stagedConfig; try { - const viteConfig = await resolveViteConfig(normalizedArgs.cwd ?? process.cwd()); + const viteConfig = await resolveViteConfig(args.cwd ?? process.cwd()); stagedConfig = viteConfig.staged; } catch (err) { // Surface real errors (syntax errors, missing imports, etc.) @@ -145,17 +142,17 @@ if (args.help) { log(' });'); process.exit(1); } - if (normalizedArgs.cwd != null) { - options.cwd = normalizedArgs.cwd; + if (args.cwd != null) { + options.cwd = args.cwd; } - if (normalizedArgs.diff != null) { - options.diff = normalizedArgs.diff; + if (args.diff != null) { + options.diff = args.diff; } - if (normalizedArgs.diffFilter != null) { - options.diffFilter = normalizedArgs.diffFilter; + if (args.diffFilter != null) { + options.diffFilter = args.diffFilter; } - if (normalizedArgs.concurrent != null) { - options.concurrent = normalizedArgs.concurrent; + if (args.concurrent != null) { + options.concurrent = args.concurrent; } const success = await lintStaged(options); diff --git a/rfcs/napi-clap-cli-args.md b/rfcs/napi-clap-cli-args.md new file mode 100644 index 0000000000..10dac3e775 --- /dev/null +++ b/rfcs/napi-clap-cli-args.md @@ -0,0 +1,616 @@ +# RFC: Parse JavaScript-backed CLI Arguments with clap + +## Summary + +Use the local `vite-plus` NAPI binding and Rust `clap` schemas to parse arguments for the five commands that JavaScript executes: + +- `create` +- `migrate` +- `config` +- `hooks` +- `staged` + +Node.js will remain the local CLI process. JavaScript will keep command dispatch, prompts, filesystem work, and calls to JavaScript libraries. Rust will own the runtime grammar, coercion, and argument validation for these commands. + +The global Rust CLI will keep forwarding their arguments without parsing them. This preserves delegation from a global binary to a project-local `vite-plus` version with a different command surface. + +## Motivation + +The local entry point in `packages/cli/src/bin.ts` sends most commands through the NAPI-backed Rust CLI. It imports the five commands in this RFC as JavaScript modules. Each module uses `mri` and TypeScript assertions to interpret its own arguments. + +That split creates two runtime argument systems: + +- Rust-backed commands use `clap`. +- JavaScript-backed commands use permissive `mri` output plus local normalization. + +[Issue #2488](https://github.com/voidzero-dev/vite-plus/issues/2488) showed a concrete failure. `mri` returns the boolean `false` for `--no-concurrent`, even when the caller declares `concurrent` as a string. The old `staged` code converted `false` to `0` and passed it to `lint-staged`. Its task queue did not start work with a concurrency limit of zero. [PR #2501](https://github.com/voidzero-dev/vite-plus/pull/2501) fixed the failure with JavaScript normalization and added regression tests. + +The fix protects `staged`, but it leaves the parser split in place. Other examples remain: + +- `vp config --no-hooks-dir` can put a boolean into a value that JavaScript treats as a path. +- Most JavaScript-backed commands accept unknown options and unused positional arguments because `mri` collects them without an error. +- `create` and `migrate` use assertions such as `as Options` and `as MigrationOptions`; those assertions do not validate runtime values. +- Each command implements negation, repeated options, and missing-value behavior on its own. + +Vite+ already ships and loads a native binding for the local CLI. Reusing its `clap` dependency removes the second runtime parser without adding a new runtime component. + +## Goals + +1. Make `clap` the runtime parser and validator for the five JavaScript-backed command grammars. +2. Give JavaScript typed values that match the generated NAPI declarations. +3. Reject unknown options, extra positional arguments, missing values, and unsupported negations before command work starts. +4. Preserve documented command behavior and the local Node.js execution model. +5. Preserve the exact `--` pass-through boundary for template arguments in `vp create`. +6. Keep help output, environment-dependent defaults, and JavaScript business rules in their present ownership layer. +7. Migrate one command at a time, starting with `staged`. + +## Non-goals + +This RFC does not: + +- move command execution, prompts, or filesystem work into Rust; +- replace the Node.js local CLI process; +- make the global Rust CLI validate project-local command options; +- route these commands through the binding's existing `run()` executor; +- replace the current `renderCliDoc()` output with clap help text; +- generate Rust schemas from TypeScript or TypeScript schemas from Rust source; +- change `-C`, `vpr`, top-level command routing, or package-manager routing; +- add a third CLI framework or a neutral schema language. + +## Ownership boundaries + +| Layer | Responsibility after this RFC | +| ------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | +| Global `vp` binary | Parse global options, select a command, find the matching local package, and forward JavaScript-command arguments as opaque strings | +| Local `packages/cli/src/bin.ts` | Apply local `-C` and `vpr` rewrites, dispatch commands, and preserve current help routing | +| `packages/cli/binding/src/js_command_args/` | Parse and validate the five JavaScript-command grammars with clap | +| JavaScript command modules | Apply runtime defaults, run prompts and filesystem operations, and adapt parsed values to JavaScript APIs | + +The global CLI must keep the Category B variants in `crates/vp_global_cli/src/cli.rs` as `Vec` forwarding contracts. A global binary can invoke an older or newer project-local `vite-plus`. If the global binary parsed the local option schema, version skew could reject an option that the selected local package supports. + +## Proposed architecture + +```text +process.argv + | + v +local Node.js CLI +packages/cli/src/bin.ts + | + | argv for one JavaScript-backed command + v +NAPI parser function +packages/cli/binding/src/js_command_args/ + | + v +clap command schema + | + +-- aliases and option boundaries + +-- value parsing and coercion + +-- explicit negation + +-- unknown-option checks + +-- positional checks + | + v +semantic Rust arguments + | + v +typed NAPI parse outcome + | + v +JavaScript command business logic +``` + +JavaScript will pass raw command arguments into one NAPI parser. It will not parse the returned values again or serialize them back into argv. + +## Rust module structure + +Add a module beside the existing binding CLI executor: + +```text +packages/cli/binding/src/js_command_args/ + mod.rs + parse.rs + create.rs + migrate.rs + config.rs + hooks.rs + staged.rs +``` + +The name `js_command_args` distinguishes these schemas from `binding/src/cli/`, which parses and executes Rust-backed local commands. + +`mod.rs` will export the NAPI functions and shared transport types. `parse.rs` will contain the common clap helper and error conversion. Each command module will contain its clap type, semantic conversion, NAPI output type, and focused tests. + +Create and migrate can flatten a private shared setup-options type when their grammar matches. They must not share fields that have different repetition or default rules. + +## Shared clap helper + +Each schema will derive `clap::Args`. A shared helper will add it to a synthetic command, prepend argv element zero, and use fallible clap APIs: + +```rust +fn try_parse_args( + bin_name: &'static str, + argv: Vec, +) -> Result +where + T: clap::Args + clap::FromArgMatches, +{ + let command = T::augment_args(clap::Command::new(bin_name)); + let mut matches = command.try_get_matches_from( + std::iter::once(bin_name.to_owned()).chain(argv), + )?; + T::from_arg_matches_mut(&mut matches) +} +``` + +The implementation can adjust ownership and iterator types. It must retain these properties: + +- All wrappers use the same parse path. +- `try_get_matches_from` receives a synthetic argv element zero. +- The helper uses `from_arg_matches_mut` or the matching fallible conversion. +- The helper does not call `get_matches`, `parse`, `exit`, or a print method. +- Tests can invoke the helper without replacing `process.argv` or capturing a process exit. + +The relevant clap APIs support this composition: [`Args::augment_args`](https://docs.rs/clap/latest/clap/trait.Args.html), [`FromArgMatches`](https://docs.rs/clap/latest/clap/trait.FromArgMatches.html), and the fallible parser methods on [`Parser`](https://docs.rs/clap/latest/clap/trait.Parser.html). + +## NAPI contract + +Export one synchronous function per command: + +```ts +parseStagedArgs(argv: string[]): ParseStagedArgsOutcome +parseConfigArgs(argv: string[]): ParseConfigArgsOutcome +parseHooksArgs(argv: string[]): ParseHooksArgsOutcome +parseMigrateArgs(argv: string[]): ParseMigrateArgsOutcome +parseCreateArgs(argv: string[]): ParseCreateArgsOutcome +``` + +Each function will return a command-specific discriminated union: + +```ts +type ParseStagedArgsOutcome = + | { status: 'ok'; value: StagedArgs } + | { status: 'help' } + | { status: 'error'; error: CliParseError }; + +interface CliParseError { + kind: string; + message: string; +} +``` + +The binding can generate this shape from a napi-rs structured enum with `status` as its discriminant. napi-rs v3 supports structured enums and emits TypeScript unions for them through [`#[napi]` attributes](https://napi.rs/docs/concepts/napi-attributes). It also emits interfaces for [`#[napi(object)]`](https://napi.rs/docs/concepts/object) output types. + +The union separates three cases: + +- `ok` contains typed, validated arguments. +- `help` tells JavaScript to render the existing help document and exit zero. +- `error` contains a stable error kind and clap's rendered diagnostic. + +The binding must map clap `DisplayHelp` to `help`. It must map other argument errors to `error`. The JavaScript command will print the Vite+ header, print the diagnostic, and exit with code 1. This retains the current exit code for invalid JavaScript-command arguments even though standalone clap applications often use code 2. + +The parser functions should return data for user input errors instead of throwing a NAPI exception. A thrown native exception will then mean that the binding contract or native conversion failed, not that a user misspelled an option. napi-rs supports both patterns, but its error documentation notes that TypeScript declarations do not encode thrown errors. See [napi-rs error handling](https://napi.rs/docs/concepts/error-handling). + +Do not return `serde_json::Value`. The generated declaration in `packages/cli/binding/index.d.cts` must list the real fields and union members. + +## Rust types and NAPI output types + +The clap type should express parser semantics. A separate, small NAPI output type may adapt Rust-only values to JavaScript: + +```text +clap StagedArgs + concurrent: Option + | + v +StagedArgsJs + concurrent?: boolean | number +``` + +That transport object does not define a second grammar. It converts a validated Rust value into the shape that JavaScript needs. + +Use these rules for every output object: + +- Use `Option` for values the user did not specify. Do not insert TTY or environment defaults in Rust. +- Expose JavaScript field names in camel case, such as `diffFilter` and `allowEmpty`. +- Use generated TypeScript declarations instead of handwritten copies in command modules. +- Keep Rust enums and newtypes inside the parser when JavaScript only needs a boolean, number, or string. +- Use an integer type that converts to a JavaScript number without precision loss. + +JavaScript may keep interfaces that describe broader command business inputs. It must not use assertions to pretend that raw parser output matches those interfaces. + +## Help handling + +Keep `renderCliDoc()` as the help renderer in this RFC. + +clap will retain its built-in `-h` and `--help` action. The fallible parser reports that action as `DisplayHelp`; the NAPI wrapper converts it to the `help` outcome and discards clap's text. JavaScript then prints the existing Vite+ help document. This also preserves the current behavior where a help flag takes precedence over a later invalid option. + +The clap schemas still need accurate names, value names, aliases, and short descriptions. A later RFC or follow-up can expose clap command metadata through NAPI and generate the `renderCliDoc()` rows. Until then, implementation PRs must update the clap schema and the JavaScript help document together. + +The `staged` help label should change from `` to `[number|boolean]` because Vite+ accepts a bare `--concurrent`. This corrects the documented value requirement; it does not change parsing behavior. + +## Strict parsing and negation + +The new schemas will reject: + +- unknown options; +- positional arguments outside each command's declared positions; +- missing option values; +- repeated scalar options unless this RFC defines a repetition rule; +- `--no-*` spellings that the schema does not declare. + +This strictness changes the permissive behavior of `mri`. The accepted command surface comes from documented options and compatibility cases, not from every property that `mri` happened to create. + +Define positive and negative forms as separate clap arguments when the public CLI supports both. Use clap overrides so the last positive or negative spelling wins. [`ArgAction`](https://docs.rs/clap/latest/clap/enum.ArgAction.html) rejects repeated scalar values by default and supports explicit override behavior. + +Apply these repetition rules: + +| Option type | Rule | +| ------------------------------ | -------------------------------------------------------------------- | +| Repeated `--agent ` | Collect names in command-line order | +| `--agent` and `--no-agent` | The last form wins; a later `--agent` starts a new enabled selection | +| Repeated `--editor ` | The last value wins, matching the existing create normalization | +| Positive and negative booleans | The last form wins | +| Other repeated scalar options | Reject as an argument conflict | + +Do not add generic negation. For example, `--no-cwd`, `--no-diff`, and `--no-hooks-dir` must fail in clap because each target expects a string. + +## Command schemas + +### `staged` + +Migrate `staged` first. It exercises aliases, explicit negation, optional values, custom value parsing, strings, and booleans. + +Represent concurrency with a semantic Rust type: + +```rust +enum Concurrent { + Enabled, + Disabled, + Limit(std::num::NonZeroU32), +} +``` + +Configure `--concurrent` with `num_args = 0..=1` and `default_missing_value = "true"`. Define `--no-concurrent` as a separate argument and make both forms override each other. clap supports optional values through `num_args` and `default_missing_value`; see [`Arg`](https://docs.rs/clap/latest/clap/builder/struct.Arg.html). + +Accept: + +```text +--concurrent +--no-concurrent +--concurrent true +--concurrent false +--concurrent=4 +-p 4 +``` + +Reject before JavaScript calls `lint-staged`: + +```text +--concurrent=0 +--concurrent=-1 +--concurrent=1.5 +--concurrent=NaN +--concurrent=4294967296 +``` + +Use `NonZeroU32` instead of `NonZeroUsize`. It produces the same range on every supported platform and converts to a precise JavaScript number. A fractional task count has no clear scheduling meaning, so this RFC treats fractions as invalid even though the normalization from PR #2501 accepts any positive finite JavaScript number. + +Return `concurrent?: boolean | number`. Return all other `lint-staged` options with camel-case names and `Option` semantics so JavaScript passes only explicit values to the programmatic API. + +The schema must include the full staged surface: + +```text +--allow-empty +-p, --concurrent [number|boolean] +--no-concurrent +--continue-on-error +--cwd +-d, --debug +--diff +--diff-filter +--fail-on-changes +--hide-partially-staged +--hide-unstaged +--no-stash +-q, --quiet +-r, --relative +--revert +-v, --verbose +-h, --help +``` + +Declare `--no-stash` as the supported stash negation. Do not infer forms such as `--no-debug` or `--stash` from `mri` behavior unless the public help and tests add those forms first. + +After this migration, `packages/cli/src/staged/bin.ts` will consume the NAPI outcome and adapt the `ok` value to `lint-staged` options. Remove `packages/cli/src/staged/args.ts` when no caller needs it. + +### `config` + +Model these options: + +```text +--hooks-dir +--hooks / --no-hooks +--agent / --no-agent +-h / --help +``` + +The positive `--hooks` and `--agent` forms preserve existing accepted input, even though each positive form matches the default today. clap must reject `--no-hooks-dir` and missing path values. + +Rust will return the raw hooks-directory string. JavaScript will keep Git lookup, path policy, lifecycle-event handling, prompts, and environment opt-outs. + +### `hooks` + +Model `enable`, `disable`, and `status` as clap subcommands. Put `--hooks-dir ` on each subcommand because the current grammar places the option after the subcommand. + +`vp hooks` and help flags will render the existing top-level hooks help. Unknown subcommands, unknown options, and extra positional arguments will fail in clap before `enable` or `disable` can mutate repository state. + +Remove `packages/cli/src/hooks/args.ts` after clap replaces `unexpectedHooksArgsError()`. + +### `migrate` + +Model: + +```text +vp migrate [PATH] [OPTIONS] +``` + +The schema will accept one optional path and reject extra positionals. It will parse `interactive`, `agent`, `editor`, `hooks`, `full`, and help options with the repetition rules in this RFC. + +The complete option surface is: + +```text +--agent / --no-agent +--editor / --no-editor +--hooks / --no-hooks +--interactive / --no-interactive +--full +-h / --help +``` + +Return the path as written. JavaScript will resolve it against `process.cwd()`. Return `interactive?: boolean`; JavaScript will calculate `parsed.interactive ?? defaultInteractive()`. Agent and editor catalog lookup will stay in JavaScript because those catalogs and compatibility aliases live there. + +### `create` + +Migrate create last: + +```text +vp create [TEMPLATE] [OPTIONS] [-- TEMPLATE_OPTIONS] +``` + +The Vite+ option surface before `--` is: + +```text +--directory +--agent / --no-agent +--editor / --no-editor +--git / --no-git +--hooks / --no-hooks +--package-manager +--approve-builds +--verbose +--interactive / --no-interactive +--list +-h / --help +``` + +Use a final clap positional with `last = true` for `templateArgs`. clap then requires the first `--` separator before this positional and treats every later token as a value, including hyphenated options and another literal `--`. This matches clap's documented [argument escape and `last` behavior](https://docs.rs/clap/latest/clap/_concepts/). + +The schema must preserve these cases: + +```text +vp create vite -- --template react-ts +vp create