refactor(cli): extract global-only commands into vp_global_only_cli - #2525
refactor(cli): extract global-only commands into vp_global_only_cli#2525jong-kyung wants to merge 6 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
e505ce3 to
1962a97
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1962a97991
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Move the env, upgrade, and implode clap definitions out of crates/vp_global_cli/src/cli.rs into a shared crate as GlobalOnlyCommand, and flatten it back into the global Commands enum. The crate also exposes is_global_only(name), derived from the enum through clap, so the names can be recognised elsewhere without a hand-maintained list that can drift from the global surface. The global CLI re-exports the moved types, so commands/env/* is unchanged. No behavior change; global PTY snapshots pass without updates.
9b536a8 to
e440f0d
Compare
jong-kyung
changed the title
|
@codex review |
|
if #2524 is merged, I'll add |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e440f0d6dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7b44924 to
dd64873
Compare
Document the shared global-only command crate next to the existing vp_pm_cli notes in merge-global-and-local-cli.md, align the command category letters with cli.rs, and update the file map in global-cli-rust-binary.md.
dd64873 to
c917708
Compare
The redrawn four-category diagram is out of scope for this PR; keep the original two-path diagram and leave the category text as the source of truth.
|
@codex review |
Replace the two-path ASCII diagram with a mermaid flowchart that shows all four routing categories, matching the list below it.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b17aa69ead
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Category A (Package Manager)**: `install`, `add`, `remove`, `update`, `dedupe`, `outdated`, `why`, `info`, `link`, `unlink`, `dlx`, `pm <subcmd>` — clap definitions and dispatch live in the shared `crates/vp_pm_cli/` crate. Both the global CLI and the local CLI binding flatten `vp_pm_cli::PackageManagerCommand` into their top-level argument parser and call `vp_pm_cli::dispatch` to run the underlying package manager (pnpm/npm/yarn/bun). The global CLI additionally intercepts `--global` for vite-plus-managed installs (`commands::env::global_install`) before delegating. | ||
| - **Category B (JavaScript)**: All other commands (`build`, `test`, `lint`, `create`, `migrate`, `--version`, etc.) — Rust uses `oxc_resolver` to find the project's local `vite-plus/dist/bin.js` and runs it. Falls back to the global installation's `dist/bin.js` if no local installation exists. The unified `bin.ts` entry point then routes to either NAPI bindings (task commands and PM commands, the latter via `vp_pm_cli::dispatch`) or rolldown-bundled modules in `dist/global/` (create, migrate, version). | ||
| - **Category B (JavaScript)**: `create`, `migrate`, `config`, `hooks`, `staged`, `--version` — Rust uses `oxc_resolver` to find the project's local `vite-plus/dist/bin.js` and runs it. Falls back to the global installation's `dist/bin.js` if no local installation exists. The unified `bin.ts` entry point then routes to either NAPI bindings (task commands and PM commands, the latter via `vp_pm_cli::dispatch`) or rolldown-bundled modules in `dist/global/` (create, migrate, version). | ||
| - **Category C (Local CLI Delegation)**: `dev`, `build`, `test`, `lint`, `fmt`, `check`, `pack`, `run`, `exec`, `preview`, `cache` — forwarded to the project-local `vite-plus` CLI through `commands::delegate`, which resolves it the same way as Category B. `lint --init` and `fmt --init`/`--migrate` are forced to the global installation. |
There was a problem hiding this comment.
The supposedly exhaustive four-category model leaves toolchain unclassified: this list ends with cache, but Commands::Toolchain remains a distinct top-level variant in crates/vp_global_cli/src/cli.rs (lines 207-220) and is dispatched through commands::toolchain::execute (lines 826-828), which either delegates locally or renders the global manifest depending on --global. Add this hybrid path to the diagram/category text or describe it as a separate category so readers do not infer that every command follows one of the four documented routes.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, but this gap predates this PR. I'll address it in a follow-up RFC update after this is merged.
cc. @fengmk2
|
This implementation is a bit over-engineered. The independently separated |
Thanks for reviewing! That make sense, I wanted to make future global-only commands impossible to forget, but a separate crate is more machinery than that problem deserves. I'll close this PR and go back to #2522 🙇♂️ |
The Command Routing section of `rfcs/merge-global-and-local-cli.md` still described the two-path model from the original merge: Category A for package-manager commands and Category B for "everything else". `crates/vp_global_cli/src/cli.rs` has since grown into three labelled blocks plus a handful of Rust-native commands, so the RFC no longer matched the code it documents. This rewrites the section to mirror `cli.rs` as it is today: - Category A (package manager), B (JS script commands: `create`, `migrate`, `config`, `hooks`, `staged`, `--version`), and C (local CLI delegation: `dev` through `cache`), matching the comment blocks in the `Commands` enum. - A separate "Rust-native global commands" group for the variants outside those blocks: `toolchain` as a hybrid that delegates locally or renders the global `toolchain.json` depending on `--global`, `env`, and the self-management pair `upgrade` and `implode`. - The `--global` interception for package-manager commands now points at `PackageManagerCommand::managed_global_command` and `commands::global`; the RFC still referenced the removed `commands::env::global_install`. - The ASCII diagram is replaced with a mermaid flowchart, which the repo already uses in `docs/` and `rfcs/core-binding-resolution.md`, so the B and C convergence on `oxc_resolver` and the `--global` branches are visible. Docs only, no code changes. Follows up on the routing gaps surfaced while reviewing #2525.
2 participants
Extract the
env,upgrade, andimplodeclap definitions out ofcrates/vp_global_cli/src/cli.rsinto a new sharedvp_global_only_clicrate asGlobalOnlyCommand, and flatten it back into the globalCommandsenum. This mirrors howvp_pm_cli::PackageManagerCommandis already shared between the global CLI and the local CLI binding.The crate also exposes
is_global_only(name), derived from the enum through clap'saugment_subcommands, so the names can be recognised elsewhere without a hand-maintained list that can drift from the global surface. This addresses the review discussion in #2522, where the local binding needed the same list for its "only available in the globalvpCLI" hint. Wiring that call into the local binding is left to #2522 itself.The global CLI re-exports the moved types, so
commands/env/*is unchanged. There is no behavior change: the global PTY snapshot suite passes without any snapshot updates, andcargo test -p vp_global_clipasses.The RFCs that describe the shared CLI layout (
merge-global-and-local-cli.md,global-cli-rust-binary.md) are updated to record the new crate.Refs #2522