fix(global): skip current packages on update#1596
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 224dd80228
ℹ️ 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".
|
Thank you! |
liangmiQwQ
left a comment
There was a problem hiding this comment.
When I made up #1477, I actually thought about a question: should we reinstall the packages installed with a different node version.
For example, you installed oxlint globally with node@21.0.0, but now you are running vp update -g with node@25.0.0. For current vp, it will be re-installed.
The reason why I have this question is different nodes can have different installing behaviors, like engine.node compatibility checks or some post-install scripts.
By the way, I'm not the team, and there are several comments I left, please use this as a reference only.
/cc @fengmk2 What do you think of it?
|
@liangmiQwQ What you mentioned should be a new feature requirement, and you can open a new issue for discussion. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 224dd80228
ℹ️ 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".
|
Addressed the review feedback in ac4ed26:
Validation run locally: cargo fmt
cargo test -p vite_global_cli global_update --no-default-features
cargo test -p vite_global_cli updates_global_package_when_registry_version_differs_from_installed_version --no-default-features
cargo test -p vite_global_cli updates_global_package_when_node_version_differs_from_target_version --no-default-features
cargo test -p vite_global_cli parse_npm_view_version --no-default-features
cargo test -p vite_global_cli test_is_local_package_spec --no-default-features
cargo test -p vite_global_cli --no-default-features --no-run
cargo clippy -p vite_global_cli --no-default-features -- -D warnings
git diff --check |
I agree on how you process this problem in this PR, really thanks for solving my issue! |
|
Agreed that changing the cross-Node reinstall policy should be discussed in #1598. I adjusted the wording/implementation intent here: this PR keeps the existing behavior for packages installed under a different Node version (they still reinstall under the current resolved Node version) and only skips redundant reinstalls when the recorded package version and Node version both match. So #1598 remains the place to decide whether that behavior should change in the future. |
Summary
vp update -ginstalls when the recorded global package version already matches the version resolved from npm.view ... version --jsonoutputs as strings, plain strings, and version arrays.Fixes #1477
Related #1476
Test Plan
cargo fmtcargo test -p vite_global_cli global_update --no-default-featurescargo test -p vite_global_cli updates_global_package_when_registry_version_differs_from_installed_version --no-default-featurescargo test -p vite_global_cli parse_npm_view_version --no-default-featurescargo test -p vite_global_cli --no-default-features --no-runcargo clippy -p vite_global_cli --no-default-features -- -D warningsNote:
cargo test -p vite_global_cli --no-default-featuresaborts locally in the existingtests::unknown_argument_detected_with_pass_as_value_hinttest with a stack overflow after the targeted tests pass.