fix(pm): keep Yarn catalog: references intact through vp up - #2432
fix(pm): keep Yarn catalog: references intact through vp up#2432fengmk2 wants to merge 2 commits into
catalog: references intact through vp up#2432Conversation
|
✅ Staging deployment successful! Preview: https://viteplus-staging.void.app/ |
Native binary sizes (
|
| Artifact | Format | Base | PR | Change |
|---|---|---|---|---|
vp (Linux x64) |
Binary | 10.67 MiB | 10.80 MiB | +132.00 KiB (+1.21%) |
vp (Linux x64) |
gzip -9 | 4.62 MiB | 4.68 MiB | +68.99 KiB (+1.46%) |
| NAPI (Linux x64) | Binary | 32.02 MiB | 32.04 MiB | +20.00 KiB (+0.06%) |
| NAPI (Linux x64) | gzip -9 | 12.61 MiB | 12.62 MiB | +10.02 KiB (+0.08%) |
vp (macOS ARM64) |
Binary | 7.98 MiB | 8.08 MiB | +97.00 KiB (+1.19%) |
vp (macOS ARM64) |
gzip -9 | 4.03 MiB | 4.09 MiB | +60.05 KiB (+1.46%) |
| NAPI (macOS ARM64) | Binary | 39.68 MiB | 39.69 MiB | +16.16 KiB (+0.04%) |
| NAPI (macOS ARM64) | gzip -9 | 16.92 MiB | 16.93 MiB | +8.45 KiB (+0.05%) |
vp (Windows x64) |
Binary | 8.55 MiB | 8.66 MiB | +119.00 KiB (+1.36%) |
vp (Windows x64) |
gzip -9 | 3.73 MiB | 3.79 MiB | +64.28 KiB (+1.68%) |
| NAPI (Windows x64) | Binary | 26.89 MiB | 26.91 MiB | +18.00 KiB (+0.07%) |
| NAPI (Windows x64) | gzip -9 | 10.68 MiB | 10.69 MiB | +7.20 KiB (+0.07%) |
| Trampoline (Windows x64) | Binary | 205.00 KiB | 205.00 KiB | 0 B (0.00%) |
| Trampoline (Windows x64) | gzip -9 | 99.00 KiB | 99.00 KiB | -1 B (-0.00%) |
| Installer (Windows x64) | Binary | 4.47 MiB | 4.47 MiB | 0 B (0.00%) |
| Installer (Windows x64) | gzip -9 | 2.09 MiB | 2.09 MiB | 0 B (0.00%) |
1d1edea to
a166bc1
Compare
a166bc1 to
172dc27
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: 172dc27beb
ℹ️ 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".
| let packages: Vec<&String> = args | ||
| .packages | ||
| .iter() | ||
| .filter(|package| !Self::skip_yarn_catalog_pinned(package.as_str(), args, diag)) |
There was a problem hiding this comment.
When a Yarn Berry user runs vp up --recursive vite for a catalog-pinned package, this filter removes vite and the following branch returns Noop, so the requested lockfile refresh never occurs. Checked Yarn 4.14.1's yarn up --help: -R,--recursive forces matching resolutions to be resolved again but does not touch manifests, so it cannot overwrite the catalog: reference and should bypass this guard.
Useful? React with 👍 / 👎.
| let yarnrc_yml_path = workspace_root.join(".yarnrc.yml"); | ||
| let Ok(content) = std::fs::read_to_string(&yarnrc_yml_path) else { |
There was a problem hiding this comment.
When a Yarn project inherits its configuration from an ancestor above the detected workspace root, or sets YARN_RC_FILENAME to use a custom filename, Yarn still resolves its catalog: references but this hardcoded path finds no catalog and returns an empty list. Confirmed with Yarn 4.14.1 that both an ancestor .yarnrc.yml and YARN_RC_FILENAME=.custom.yml supply the active catalog; in either case vp up <name> therefore forwards the bare name and permits the manifest rewrite this guard is intended to prevent.
Useful? React with 👍 / 👎.
| collect_mapping_keys(doc.get("catalog"), &mut names); | ||
| if let Some(catalogs) = doc.get("catalogs").and_then(serde_yaml::Value::as_mapping) { | ||
| for named_catalog in catalogs.values() { | ||
| collect_mapping_keys(Some(named_catalog), &mut names); | ||
| } |
There was a problem hiding this comment.
When .yarnrc.yml retains a catalog entry that no workspace manifest references, this code still treats the package as pinned, so vp up react is skipped even if the actual dependency is an ordinary semver range that should be updated. Catalog mappings only provide ranges for dependencies that explicitly use catalog: or catalog:<name>; merely appearing as a mapping key does not pin every declaration of that package, so the guard should be based on catalog references in the affected manifests rather than every configured key.
Useful? React with 👍 / 👎.
The Yarn variant of #2309: `yarn up <name>` rewrites the manifest spec of every named package, so a migrated project's `vite: "catalog:"` became `vite: "^8.2.1"` (upstream Vite) and `vite-plus: "catalog:"` became a concrete range. Verified on Yarn 4.12.0 and 4.18.0; Yarn has no upstream fix, and `yarn up 'name@catalog:'` is rejected by the resolver, so the guard lives in vp. Dispatch reads the catalog package names from the workspace root's .yarnrc.yml and hands them to resolution as data. The Yarn Berry update resolver skips catalog-pinned bare names with a warning and resolves to a no-op when nothing else was requested. A descriptor with an explicit range (`vp up vite@^8`) still passes through. npm needs no change: `npm update` never writes package.json.
Mirrors command_update_catalog_protocol_pnpm: migrate a minimal project to the Yarn catalog layout, then `vp up vite vite-plus` must skip both catalog-pinned names with a warning and leave package.json and the .yarnrc.yml catalog untouched.
172dc27 to
6d7ba3e
Compare
1 participant
The Yarn variant of #2309, stacked on #2425. Under Yarn 4.10+ a migrated project references the toolchain pins with
catalog:.yarn up <name>rewrites the manifest spec of every named package, sovp up viteturned"vite": "catalog:"into"vite": "^8.2.1"(upstream Vite) andvp up vite-pluswrote a concrete range. The catalog reference is destroyed either way. Verified on Yarn 4.12.0 and 4.18.0: Yarn has no upstream fix, andyarn up 'vite@catalog:'fails with "isn't supported by any available resolver", so the guard lives in vp.Dispatch now reads the catalog package names from the workspace root's
.yarnrc.yml(catalogplus every namedcatalogsentry) and hands them to command resolution as data, so resolution stays free of filesystem access. The Yarn Berry update resolver skips catalog-pinned bare names with a warning and resolves to a no-op when nothing else was requested:A descriptor with an explicit range (
vp up vite@^8) still passes through: that spelling states the intent to leave the catalog. Glob patterns also pass through. Yarn 1 has no catalogs and keeps its behavior.npm needs no fix.
npm updatenever writes package.json; verified on a migrated npm project with plain, named, and toolchain-named updates.The PTY snapshot
command_update_catalog_protocol_yarnmirrors the pnpm fixture from #2425: migrate a minimal project, runvp up vite vite-plus, and assert both manifests keep theircatalog:references.docs/guide/upgrade.mdanddocs/guide/install.mddocument the skip behavior. Verified withcargo test -p vp_pm_cli, crate clippy and fmt, and the threecommand_update_catalog_protocol_*snapshot cases.