fix(migrate): keep catalog: specs intact through vp up under pnpm - #2425
fix(migrate): keep catalog: specs intact through vp up under pnpm#2425fengmk2 wants to merge 9 commits into
catalog: specs intact through vp up under pnpm#2425Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
✅ Staging deployment successful! Preview: https://viteplus-staging.void.app/ |
19982a8 to
3d8f17b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d8f17baf2
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4321a19ebe
ℹ️ 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".
47cc4bb to
8b2ce1d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
pnpm applies overrides through a read-package hook that replaces the declared spec on every manifest, importers included, before resolution runs. A bare override key carries no range, and pnpm treats "no range" as "matches every declared spec", `catalog:` included. An importer declaring `vite: "catalog:"` therefore loses its catalog provenance during resolution, so `pnpm update` writes the resolved core alias back into its package.json (`vite: "npm:@voidzero-dev/vite-plus-core@0.2.7"`). Range-qualify the managed pnpm override keys (`vite@*`, `vitest@*`). `*` is a valid range and intersects every real semver range, which is what the transitive and peer `vite` declarations the override exists for always use, while `catalog:` is not a valid range and so never matches. Nothing changes about what gets installed: an importer referencing the catalog already resolves to the aliased core through its catalog entry. Migration re-keys a project still carrying the bare key, preserving its `catalog:<name>` choice, and pending-detection reads a bare managed key as unsatisfied so one `vp migrate` repairs an existing project. npm/bun `overrides` and yarn `resolutions` keep bare keys; they have no `catalog:` importer specs to lose. Closes #2309
…sion The fixture hand-wrote `vite: npm:@voidzero-dev/vite-plus-core@latest` in the catalog, so `vp up` had a real version bump to perform and the clobber read as a side effect of that resolution. The reported case pins an exact version, where the update is a NO-OP and the catalog reference is still destroyed, which is both the reporter's shape and the sharper repro. Let `vp migrate` write the pinned version instead of hard-coding one that would churn each release, then rewind the override key to the pre-fix bare spelling with `vpt replace-file-content` (which also fails loudly if migrate ever stops writing the ranged key). `vp up` now reports "Already up to date" and still rewrites `vite: "catalog:"` to the pinned alias. Reverts the redaction widening from the previous commit: with an exact pin no snapshot produces the `@^0.2.8` range form any more.
These 11 fixtures timed out on the local recording pass (slow managed Node/package-manager downloads), so they kept the pre-#2309 bare `vite` override key and failed CI on all three platforms. Every diff is the key rename; no behavior changed.
pnpm 12 fixes the clobber upstream: on 12.0.0-rc.3 a bare override key no longer strips a catalog: importer spec, verified by A/B on identical config (pnpm 11.20.0 rewrites it, 12.0.0-rc.3 does not). The range-qualified key vite-plus writes stays correct there, and still redirects an incompatible transitive edge, so it is not merely a pnpm 9-11 workaround. The vp up screen is omitted: pnpm 12's update summary reports a package-count delta that churns with the bundled dependency graph.
Apply ASD-STE100 style to the comments and guide text added by this branch: one instruction per sentence, active voice, no ellipsis, and sentences under 25 words. The longest sentence was 52 words. Also removes the em dashes these additions introduced. Comments and documentation only. No code changed.
Four gaps in the range-qualified override key change. The monorepo root writer was missed. `rewriteMonorepo` routes root pnpm settings through `rewriteRootWorkspacePackageJson`, not the standalone writer, and its pre-10.6.2 branch still spread bare `...managed` keys into `pkg.pnpm.overrides`. Those workspace-wide bare keys kept matching child `catalog:` declarations, so `vp up` could still replace them. Both writers now share one `mergeManagedPnpmOverrides` helper, which is what let the paths drift apart in the first place. That helper also carries a legacy bare key's `catalog:<name>` value onto the ranged key. The package.json paths previously replaced it with the concrete managed alias, which disconnected the override from the user's named catalog and contradicted the documented preservation behaviour. A `file:` managed spec still wins, since force-override mode has no catalog. `overridesSatisfyVitePlus` read only the ranged key in `pnpm-ranged` mode, so a map holding BOTH `vite@*` and a legacy bare `vite` reported satisfied. The fast path then skipped the rewrite and left the clobbering key active. Any remaining bare managed key now reports unsatisfied. The managed-vitest sweep was over-broad. `removeManagedVitestEntry` and `removeYamlMapVitestEntry` deleted `vitest@*` from every sink, but only pnpm overrides use that spelling. npm also accepts a range in an override key, so a user-authored `"vitest@*"` in npm `overrides` was silently deleted. Both now take a key style and sweep the ranged spelling for pnpm sinks only. Each fix has a unit test that fails without it.
Codex review on 4321a19: `vite@*` does not match a declaration pinning an exact prerelease. pnpm compares with semver.intersects(declaredSpec, keyRange), which is asymmetric for prereleases: intersects('8.0.0-beta.18', '*') is false while the reverse is true. My earlier justification checked the wrong argument order. Verified on pnpm 11.20.0: a bare key redirects an exact-prerelease declaration, the ranged key does not. No range string avoids it, because node-semver only admits a prerelease when a comparator carries the same version tuple. A prerelease RANGE (^8.0.0-beta.1) still matches, and pnpm 12.0.0-rc.3 matches the exact form too, so the gap is pnpm 9-11 only. A bare key would close it but is what clobbers catalog:, so below pnpm 12 the two cannot both hold. Corrects the comments and guide text that claimed the range keeps the override on every valid semver range.
The pnpm 9 to 11 exact-prerelease gap stays in the `pnpmOverrideKey` comment. The migrate-rules and upgrade guides drop it per review. Apply STE style to the prose added after 98a2fd3: split sentences over the length limits, keep one statement per sentence, use active voice, and remove the em dash from the pnpm override bullet.
8b2ce1d to
8eb791c
Compare
Apply the /simplify review findings on this branch's code: - Drop the `deleteYamlMapKey` helper. Its premise was wrong: yaml 2.9.0's `YAMLMap.delete` resolves a plain string key through `findPair`, verified against the installed package. The YAML sink now deletes the bare key directly, with the same missing-map guard. - Probe the two managed `vitest` key spellings directly instead of scanning every sink entry through a predicate; `managedVitestOverrideKeys` now owns the spelling list and `isManagedVitestOverrideKey` is gone. - Fold the force-override `vite-plus` pin into `mergeManagedPnpmOverrides`, so both package.json `pnpm.overrides` writers really do go through the one helper and the twin call-site blocks collapse to one line each. - Restructure `ensureOverrideEntries` to move-then-check: the bare key moves onto the ranged key first, which removes the trailing re-materialize branch and its cross-branch `changed` invariant. - Write the lingering-vitest check so the ranged spelling only appears under `keyStyle === 'pnpm-ranged'`; the old disjunction read the same key twice for bare sinks.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
Registry bridge build (
|
| Package | Version |
|---|---|
vite-plus |
0.0.0-commit.ccf26fd42e409003faf2e0031531261c59303851 |
@voidzero-dev/vite-plus-core |
0.0.0-commit.ccf26fd42e409003faf2e0031531261c59303851 |
Install the Vite+ CLI built from this commit, then migrate a project:
# macOS / Linux
curl -fsSL https://vite.plus | VP_PR_VERSION=2425 bash# Windows (PowerShell)
$env:VP_PR_VERSION="2425"; irm https://vite.plus/ps1 | iexAfter installing, upgrade the current project's vite-plus to this test build with:
vp migrateOr point your package manager at the bridge registry https://registry-bridge.viteplus.dev/:
| Package manager | Registry config |
|---|---|
| npm / pnpm / Bun | .npmrc: registry=https://registry-bridge.viteplus.dev/ |
| Yarn (v2+) | .yarnrc.yml: npmRegistryServer: "https://registry-bridge.viteplus.dev/" |
Then pin the build (vite aliases to vite-plus-core; pnpm can use a catalog, npm an overrides entry):
{
"devDependencies": {
"vite-plus": "0.0.0-commit.ccf26fd42e409003faf2e0031531261c59303851",
"vite": "npm:@voidzero-dev/vite-plus-core@0.0.0-commit.ccf26fd42e409003faf2e0031531261c59303851"
}
}
🐳 Docker preview imageBuilt from this PR's registry bridge build:
# remove any stale local copy from a previous run, then pull fresh
docker rmi ghcr.io/voidzero-dev/vite-plus:pr-2425 2>/dev/null; docker pull ghcr.io/voidzero-dev/vite-plus:pr-2425Quick check: docker run --rm ghcr.io/voidzero-dev/vite-plus:pr-2425 vp --versionSee docs/guide/docker.md for usage. |
Fixes #2309.
vp uprewrote"vite": "catalog:"to the concretenpm:@voidzero-dev/vite-plus-core@xalias. The catalog already pins the version, so the update was a no-op that destroyed the reference.vp uppasses through topnpm update. pnpm applies an override by replacing the declared spec on every manifest, and a bare override key matches any spec,catalog:included.The fix range-qualifies the managed override keys:
vite@*andvitest@*. The range matches the semver ranges that transitive and peer declarations use, and never matchescatalog:, which is not a valid range. Onevp migratere-keys existing projects.Caveats: pnpm does not document this override-key behavior. Verified on pnpm 10.34.4 through 12.0.0-rc.3 (pnpm 12 also fixes the clobber upstream); the snapshot fixtures are the tripwire if pnpm changes it. On pnpm 9 to 11 the ranged key skips a declaration that pins an exact prerelease; the
pnpmOverrideKeydoc comment records that gap.Repro: https://github.com/why-reproductions-are-required/vite-plus-up-catalog-protocol-2309