fix(global): keep the installed version spec on global update#2249
fix(global): keep the installed version spec on global update#2249TheAlexLichter wants to merge 3 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
✅ Staging deployment successful! Preview: https://viteplus-staging.void.app/ |
44cccc1 to
2776009
Compare
Native binary sizes (
|
| Artifact | Format | Base | PR | Change |
|---|---|---|---|---|
vp (Linux x64) |
Binary | 10.31 MiB | 10.33 MiB | +24.00 KiB (+0.23%) |
vp (Linux x64) |
gzip -9 | 4.42 MiB | 4.43 MiB | +9.74 KiB (+0.21%) |
| NAPI (Linux x64) | Binary | 33.07 MiB | 33.07 MiB | 0 B (0.00%) |
| NAPI (Linux x64) | gzip -9 | 12.72 MiB | 12.72 MiB | 0 B (0.00%) |
vp (macOS ARM64) |
Binary | 7.64 MiB | 7.65 MiB | +16.16 KiB (+0.21%) |
vp (macOS ARM64) |
gzip -9 | 3.84 MiB | 3.85 MiB | +13.01 KiB (+0.33%) |
| NAPI (macOS ARM64) | Binary | 40.50 MiB | 40.50 MiB | 0 B (0.00%) |
| NAPI (macOS ARM64) | gzip -9 | 16.97 MiB | 16.97 MiB | +11 B (+0.00%) |
vp (Windows x64) |
Binary | 8.35 MiB | 8.37 MiB | +26.00 KiB (+0.30%) |
vp (Windows x64) |
gzip -9 | 3.64 MiB | 3.65 MiB | +13.53 KiB (+0.36%) |
| NAPI (Windows x64) | Binary | 27.51 MiB | 27.51 MiB | 0 B (0.00%) |
| NAPI (Windows x64) | gzip -9 | 10.70 MiB | 10.70 MiB | +2 B (+0.00%) |
| Trampoline (Windows x64) | Binary | 203.00 KiB | 203.00 KiB | 0 B (0.00%) |
| Trampoline (Windows x64) | gzip -9 | 97.91 KiB | 97.91 KiB | 0 B (0.00%) |
| Installer (Windows x64) | Binary | 4.44 MiB | 4.44 MiB | 0 B (0.00%) |
| Installer (Windows x64) | gzip -9 | 2.08 MiB | 2.08 MiB | 0 B (0.00%) |
|
@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: 2776009380
ℹ️ 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".
| .filter(|package| package.wanted != package.current) | ||
| .map(|package| package.spec.unwrap_or(package.name)), |
There was a problem hiding this comment.
When an explicit replacement spec resolves to the currently installed version, this filter drops the package, so the install path never records the new spec. For example, if a package installed from nightly is currently 1.0.0, vp update -g pkg@1.0.0 reports success but leaves versionSpec: "nightly"; a later bare update still follows nightly instead of holding the requested pin. This contradicts the documented switching behavior in docs/guide/install.md:72, so explicit specs that differ from the stored spec need to be persisted even without a version change.
Useful? React with 👍 / 👎.
| let (wanted, latest) = match (resolve(&wanted_key), resolve(&package.name)) { | ||
| (Ok(wanted), Ok(latest)) => (wanted, latest), | ||
| (Err(error), _) | (_, Err(error)) => { | ||
| failures.push(error); | ||
| continue; |
There was a problem hiding this comment.
If the recorded tag or range resolves successfully but the package's bare latest lookup fails, this combined match discards the valid wanted version and vp update -g skips the package. This can occur after a publisher removes latest—npm's dist-tag --help explicitly supports npm dist-tag rm <package-spec> <tag>—while a channel such as nightly remains valid and has a newer release. The update path only needs wanted, so failure to obtain the informational latest value should not block following the recorded channel.
Useful? React with 👍 / 👎.
| for failure in &report.failures { | ||
| output::warn(&format!("{failure}; skipping")); | ||
| } |
There was a problem hiding this comment.
When a registry lookup fails, this path only prints a warning and then discards the failure state. If the failed package is the only requested package, to_update remains empty, so the command prints All global packages are up to date. and exits successfully even though it could not determine or install the wanted version; the new deleted-tag snapshot demonstrates this exact result. The previous error_on_fail update path propagated such failures, and scripts invoking vp update -g now have no exit-status signal that an update was skipped.
Useful? React with 👍 / 👎.
| if version_spec.is_empty() | ||
| || version_spec == "latest" | ||
| || version_spec.contains(':') | ||
| || version_spec.contains('/') | ||
| { | ||
| None | ||
| } else { | ||
| Some(version_spec) |
There was a problem hiding this comment.
A valid npm spec such as pkg@archive.tgz is a local file spec, but it contains neither : nor /, so this function records archive.tgz as though it were a registry tag or range. Subsequent bare vp update -g pkg calls therefore run the registry lookup against pkg@archive.tgz, which is resolved as a local file relative to the update-time working directory and usually fails instead of following the intended implicit latest behavior for non-registry installs. Use npm package-spec classification rather than these character checks, or at least exclude tarball suffixes.
Useful? React with 👍 / 👎.
| } else if latest { | ||
| None | ||
| } else { | ||
| Some(metadata.update_spec()) |
There was a problem hiding this comment.
Routing recorded ranges through npm view <pkg>@<range> version --json does not reliably produce the highest satisfying version: npm documents that a range prints data for every matching version, while parse_npm_view_version at lines 288-292 simply selects the last array element without sorting by SemVer. If a lower version was published after a higher one, vp outdated -g reports the lower release as wanted, and vp update -g repeatedly schedules the package even though npm installation resolves the same range to the actual highest version. The range result must be selected with SemVer precedence rather than registry/output order.
Useful? React with 👍 / 👎.
|
|
|
@TheAlexLichter Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
2 similar comments
|
@TheAlexLichter Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
@TheAlexLichter Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
Codex noooo |
oh, what happened? |
3 participants
Resolves #2247
At the moment, when
vpinstalls global packages from a dist-tag or range (for examplevp i pkg@nightly -g) it only records the resolved version, not the actual tag or range.That means,
vp up -g(re-)resolved every package againstlatest, silently replacing a tag likenightlywith an older stable release.This PR causes that global installs now record the version spec, and update/outdated resolve within it, meaning tags follow their channel, ranges stay in range, pins hold.
vp outdated -greports Wanted vs Latest and warns if there is an unresolvable spec (e.g. a deleted dist tag).vp update -g --latestexplicitly moves packages back tolatestand clears the recorded specs.