Skip to content

Commit 50a6b75

Browse files
committed
fix(cli): address Codex review round 3
- vpr -C <dir> <task> works on both entry points: the global vpr dispatch consumes the flag before delegating to run, and the local bin/vpr shim inserts 'run' after the -C tokens - the workspace root needs a stronger runnable signal than members: a shared root vite.config.ts (lint/fmt/tasks) no longer makes the root an app target for dev/build/preview; root index.html still does - pack ranking requires an explicitly declared pack block (get_declared), so a spread-only config cannot be auto-selected into a tsdown run; FieldMap::contains is gone with its last caller - add --assetsDir/--assetsInlineLimit/--filter to the required-value flag list per vite's CLI docs Regression fixtures: a shared config at the listing fixture's root and a spread-only package in pack_default_entry, both pinned by unchanged snapshots.
1 parent be1826f commit 50a6b75

9 files changed

Lines changed: 74 additions & 40 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// Shared workspace config (lint/fmt style): must NOT make the root an app target.
2+
export default {
3+
lint: {},
4+
};
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{ "name": "spread-only", "private": true }
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
const base = {};
2+
3+
export default {
4+
...base,
5+
};

crates/vite_global_cli/src/commands/vpr.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,32 @@ use vite_shared::output;
1010
///
1111
/// Called from shim dispatch when `argv[0]` is `vpr`.
1212
pub async fn execute_vpr(args: &[String], cwd: &AbsolutePath) -> i32 {
13+
// `vpr -C <dir> <task>` mirrors `vp -C <dir> run <task>`: consume the
14+
// global flag before treating the rest as run arguments.
15+
let mut args = args;
16+
let mut cwd_buf = cwd.to_absolute_path_buf();
17+
if let Some((dir, consumed)) = crate::parse_leading_chdir(args) {
18+
cwd_buf = cwd_buf.join(&dir).clean();
19+
if !cwd_buf.as_path().is_dir() {
20+
output::raw_stderr(&format!("directory not found: {dir}"));
21+
return 1;
22+
}
23+
if std::env::set_current_dir(cwd_buf.as_path()).is_err() {
24+
output::error(&format!("Failed to change directory to {dir}"));
25+
return 1;
26+
}
27+
#[cfg(unix)]
28+
// SAFETY: single-threaded startup, before any command logic runs.
29+
unsafe {
30+
std::env::set_var("PWD", cwd_buf.as_path());
31+
}
32+
args = &args[consumed..];
33+
}
34+
1335
if crate::help::maybe_print_unified_delegate_help("run", args, true) {
1436
return 0;
1537
}
1638

17-
let cwd_buf = cwd.to_absolute_path_buf();
1839
match super::delegate::execute(cwd_buf, "run", args).await {
1940
Ok(status) => status.code().unwrap_or(1),
2041
Err(e) => {

crates/vite_global_cli/src/main.rs

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,14 @@ use crate::cli::{
3939
try_parse_args_from_with_options,
4040
};
4141

42-
/// Parse a leading `-C <dir>` / `-C=<dir>` / `-C<dir>` as the first user
43-
/// argument, mirroring clap's short-option grammar (and bin.ts on the local
44-
/// path). Returns the directory and how many argv tokens it consumed.
45-
fn parse_leading_chdir(args: &[String]) -> Option<(String, usize)> {
46-
let first = args.get(1)?;
42+
/// Parse a leading `-C <dir>` / `-C=<dir>` / `-C<dir>` at the start of the
43+
/// user arguments (argv without the binary name), mirroring clap's
44+
/// short-option grammar (and bin.ts on the local path). Returns the
45+
/// directory and how many tokens it consumed.
46+
pub(crate) fn parse_leading_chdir(user_args: &[String]) -> Option<(String, usize)> {
47+
let first = user_args.first()?;
4748
if first == "-C" {
48-
return args.get(2).map(|dir| (dir.clone(), 2));
49+
return user_args.get(1).map(|dir| (dir.clone(), 2));
4950
}
5051
if let Some(rest) = first.strip_prefix("-C") {
5152
let dir = rest.strip_prefix('=').unwrap_or(rest);
@@ -329,7 +330,7 @@ async fn main() -> ExitCode {
329330
// the target directory here too: cwd-sensitive completers (run tasks)
330331
// should suggest from <dir>, where the completed command will run.
331332
if env::var_os("VP_COMPLETE").is_some()
332-
&& let Some((dir, _)) = parse_leading_chdir(&args)
333+
&& let Some((dir, _)) = parse_leading_chdir(&args[1..])
333334
&& let Ok(current) = vite_path::current_dir()
334335
{
335336
let target = current.join(&dir).clean();
@@ -365,7 +366,7 @@ async fn main() -> ExitCode {
365366
// behave exactly as if vp had been started in <dir>. Setting the process
366367
// cwd (before any command logic runs) keeps `vite_path::current_dir()`
367368
// callers deep inside command implementations equivalent to the cd form.
368-
if let Some((dir, consumed)) = parse_leading_chdir(&args) {
369+
if let Some((dir, consumed)) = parse_leading_chdir(&args[1..]) {
369370
cwd = cwd.join(&dir).clean();
370371
if !cwd.as_path().is_dir() {
371372
output::raw_stderr(&format!("directory not found: {dir}"));
@@ -527,13 +528,13 @@ mod tests {
527528
fn parse_leading_chdir_accepts_all_clap_short_forms() {
528529
// `main` consumes these before alias normalization and the picker, so
529530
// `vp -C dir node --version` normalizes like `cd dir && vp node ...`.
530-
assert_eq!(parse_leading_chdir(&s(&["vp", "-C", "apps/web", "dev"])), some_dir(2));
531-
assert_eq!(parse_leading_chdir(&s(&["vp", "-Capps/web", "dev"])), some_dir(1));
532-
assert_eq!(parse_leading_chdir(&s(&["vp", "-C=apps/web", "dev"])), some_dir(1));
531+
assert_eq!(parse_leading_chdir(&s(&["-C", "apps/web", "dev"])), some_dir(2));
532+
assert_eq!(parse_leading_chdir(&s(&["-Capps/web", "dev"])), some_dir(1));
533+
assert_eq!(parse_leading_chdir(&s(&["-C=apps/web", "dev"])), some_dir(1));
533534
// Missing or empty value falls through to clap's own error.
534-
assert_eq!(parse_leading_chdir(&s(&["vp", "-C"])), None);
535-
assert_eq!(parse_leading_chdir(&s(&["vp", "-C="])), None);
536-
assert_eq!(parse_leading_chdir(&s(&["vp", "dev"])), None);
535+
assert_eq!(parse_leading_chdir(&s(&["-C"])), None);
536+
assert_eq!(parse_leading_chdir(&s(&["-C="])), None);
537+
assert_eq!(parse_leading_chdir(&s(&["dev"])), None);
537538
}
538539

539540
fn some_dir(consumed: usize) -> Option<(String, usize)> {

crates/vite_static_config/src/lib.rs

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -71,17 +71,6 @@ impl FieldMap {
7171
}
7272
}
7373

74-
/// Presence check with the same semantics as [`get`](Self::get) returning
75-
/// `Some`, without cloning the field's value. In an open map (spread or
76-
/// computed keys) every key may exist and therefore counts as present.
77-
#[must_use]
78-
pub fn contains(&self, key: &str) -> bool {
79-
match &self.0 {
80-
FieldMapInner::Closed(map) => map.contains_key(key),
81-
FieldMapInner::Open(_) => true,
82-
}
83-
}
84-
8574
/// Like [`get`](Self::get), but only for fields the config explicitly
8675
/// declares. Unlike `get`, an open map (spread, computed keys, or an
8776
/// unanalyzable config) does NOT report undeclared keys as `NonStatic`:

packages/cli/bin/vpr

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,9 @@ if (module.enableCompileCache) {
55
module.enableCompileCache();
66
}
77

8-
process.argv.splice(2, 0, 'run');
8+
// Insert `run` after a leading global `-C <dir>` so `vpr -C <dir> <task>`
9+
// behaves like `vp -C <dir> run <task>` (bin.ts consumes -C before dispatch).
10+
const first = process.argv[2];
11+
const chdirTokens = first === '-C' ? 2 : first?.startsWith('-C') && first.length > 2 ? 1 : 0;
12+
process.argv.splice(2 + chdirTokens, 0, 'run');
913
await import('../dist/bin.js');

packages/cli/binding/src/cli/app_target.rs

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ const VALUE_TAKING_FLAGS: &[&str] = &[
5959
"--port",
6060
"--base",
6161
"--outDir",
62+
"--assetsDir",
63+
"--assetsInlineLimit",
64+
"--filter",
6265
"-d",
6366
"--out-dir",
6467
"--target",
@@ -90,17 +93,22 @@ fn is_bare(args: &[String]) -> bool {
9093
/// Used for ordering and single-candidate auto-selection, never for hiding.
9194
/// The rules are documented in rfcs/cwd-flag.md ("The likely-runnable
9295
/// heuristic"); keep both in sync.
93-
fn looks_runnable(dir: &AbsolutePathBuf, command: &str) -> bool {
96+
///
97+
/// The workspace root needs a stronger signal than member packages: a shared
98+
/// root `vite.config.ts` (lint/fmt/tasks) is the normal monorepo setup and
99+
/// must not make the root look like an app, or auto-select would run the
100+
/// silent root build this feature exists to prevent.
101+
fn looks_runnable(dir: &AbsolutePathBuf, command: &str, is_root: bool) -> bool {
94102
match command {
95-
// Bare `vp pack` succeeds when the config declares a `pack` block or
96-
// tsdown's default entry exists; a Vite config without `pack` does
97-
// not make a package packable. `contains` deliberately counts configs
98-
// with spreads (`pack` may exist behind them): the right direction
99-
// for a never-hide ranking signal.
103+
// Bare `vp pack` succeeds when the config explicitly declares a
104+
// `pack` block or tsdown's default entry exists. A spread that only
105+
// might contain `pack` does not count: auto-select acts on this
106+
// signal, so a false positive runs tsdown in a non-packable package.
100107
"pack" => {
101-
vite_static_config::resolve_static_config(dir).contains("pack")
108+
vite_static_config::resolve_static_config(dir).get_declared("pack").is_some()
102109
|| dir.as_path().join("src/index.ts").is_file()
103110
}
111+
_ if is_root => dir.as_path().join("index.html").is_file(),
104112
_ => vite_static_config::has_config_file(dir) || dir.as_path().join("index.html").is_file(),
105113
}
106114
}
@@ -217,8 +225,8 @@ pub(super) fn resolve_app_target(
217225
.node_weights()
218226
.filter_map(|info| {
219227
let absolute = info.absolute_path.to_absolute_path_buf();
220-
let runnable = looks_runnable(&absolute, command);
221228
let is_root = info.path.as_str().is_empty();
229+
let runnable = looks_runnable(&absolute, command, is_root);
222230
// The root itself is a valid target only when it looks runnable;
223231
// `.` keeps the -C hint and the selection working there.
224232
if is_root && !runnable {
@@ -284,6 +292,7 @@ mod tests {
284292
// Known value-taking flags consume their value.
285293
assert!(is_bare(&to_args(&["--port", "3000"])));
286294
assert!(is_bare(&to_args(&["--mode", "production", "--minify"])));
295+
assert!(is_bare(&to_args(&["--assetsDir", "assets"])));
287296
assert!(is_bare(&to_args(&["--port=3000"])));
288297
// A token after an unknown or optional-value flag is ambiguous with a
289298
// positional target, so it conservatively counts as non-bare.

rfcs/cwd-flag.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -278,17 +278,17 @@ The global binary also resolves the local `vite-plus` install from `<dir>`, matc
278278

279279
- One row per workspace package: name plus relative path. Nothing is filtered out; likely-runnable packages (rules below) rank first, then by path, so apps surface at the top while everything stays searchable.
280280
- Fuzzy search over name and path via `vite_select::fuzzy_match`, paging identical to the task picker.
281-
- A runnable workspace root appears as a `(workspace root)` entry, keeping today's "run at root" behavior one keystroke away.
281+
- A runnable workspace root appears as a `.` entry, keeping today's "run at root" behavior one keystroke away. The root needs a stronger signal than member packages: an `index.html` for `dev`/`build`/`preview` (a shared root config for lint/fmt/tasks is the normal monorepo setup and does not make the root an app), and the usual explicit-`pack`-or-default-entry rule for `pack`.
282282
- With exactly one likely-runnable package, the picker auto-selects it, printing only the `Selected package:` line and the tip.
283283

284284
### The likely-runnable heuristic
285285

286286
Used only for ranking and single-candidate auto-select, never to hide a package: a misjudged package still appears in the picker and listing, just lower. Judged per package directory from file existence and static config extraction; nothing is executed, and parent directories never count.
287287

288-
| Command | A package is likely runnable when |
289-
| --------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
290-
| `dev` / `build` / `preview` | its directory directly contains one of Vite's config file names (`vite.config.{js,mjs,ts,cjs,mts,cts}`, the exact list Vite probes), **or** an `index.html` at the package root (Vite's default app entry) |
291-
| `pack` | its `vite.config.*` declares a `pack` block (read via static extraction; a Vite config without `pack` does not count), **or** `src/index.ts` exists (tsdown's only default entry) |
288+
| Command | A package is likely runnable when |
289+
| --------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
290+
| `dev` / `build` / `preview` | its directory directly contains one of Vite's config file names (`vite.config.{js,mjs,ts,cjs,mts,cts}`, the exact list Vite probes), **or** an `index.html` at the package root (Vite's default app entry) |
291+
| `pack` | its `vite.config.*` explicitly declares a `pack` block (read via static extraction; neither a config without `pack` nor one that merely might contain it behind a spread counts), **or** `src/index.ts` exists (tsdown's only default entry) |
292292

293293
Both file-based signals are upstream defaults, not vp inventions: `index.html` at the project root is Vite's entry point ([index.html and Project Root](https://vite.dev/guide/#index-html-and-project-root)), the config file names are the list Vite resolves ([Configuring Vite](https://vite.dev/config/), mirrored by `vite_static_config::CONFIG_FILE_NAMES` with the upstream source link), and `src/index.ts` is tsdown's default entry when none is configured ([tsdown Entry](https://tsdown.dev/options/entry); `src/features/entry.ts` in tsdown resolves exactly this one path).
294294

0 commit comments

Comments
 (0)