fix(migrate): tolerate nested npm overrides during bootstrap detection - #2267
fix(migrate): tolerate nested npm overrides during bootstrap detection#2267charpeni wants to merge 2 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9f15758d9
ℹ️ 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".
| if (typeof spec !== 'string' || !spec) { | ||
| return false; |
There was a problem hiding this comment.
Preserve nested override children when adding the managed alias
When overrides.vite uses npm's long object form, returning false here makes ensureOverrideEntries replace the entire object with a string, silently deleting child overrides such as the test's rollup pin. It also misclassifies { ".": "npm:@voidzero-dev/vite-plus-core@latest", "rollup": "^4" }, even though the managed alias is already satisfied. The npm 11.4.2 package-json documentation explicitly defines this form as supporting both the package itself via "." and its children, so migration should preserve the object and inspect or set its "." member rather than collapsing it.
Useful? React with 👍 / 👎.
Summary
overrideSpecSatisfiesVitePlusso a non-string override value never reachesspec.startsWith(), which crashedvp migratewithTypeError: spec.startsWith is not a function{"overrides": {"vite": {"rollup": "..."}}}) as not satisfying the managed override, so the bootstrap stays pending andensureOverrideEntrieswrites the managed aliasviteandvitest)Why
npm supports nested
overridesobjects (a user override scoped under a dependency), butdetectVitePlusBootstrapPendingassumed every override value is a string. A valid package.json with a nested override under a managed key crashed migration during bootstrap detection.A nested object never aliases the dependency itself, so it cannot satisfy the managed Vite+ override. Returning
falsemakes detection report the bootstrap as pending, and the existing rewrite then sets the managed spec — after which detection converges. The single guard covers every caller (overridesSatisfyVitePlusacross npm/bun/pnpm/yarn sources,npmVitePlusManagedDependenciesPending, andensureOverrideEntries), and also hardens against other non-string values, such as a numeric scalar read frompnpm-workspace.yamloverrides.When the project does not use vitest directly, a user override scoped under
vitestis left intact, matching the existing contract documented inremoveManagedVitestEntry. Note that under a managed key the rewrite replaces the nested object with the managed string spec, dropping any user sub-overrides scoped beneath it (e.g. avite > rolluppin); preserving those via npm's nested"."syntax is left as a possible follow-up.Fixes #2002.
Validation
pnpm -F vite-plus exec vitest run src/migration/__tests__/migrator.spec.ts -t 'tolerates nested npm override objects'— fails without the fix with the exact reported crash (TypeError: spec.startsWith is not a functionatoverrideSpecSatisfiesVitePlus, reached viadetectVitePlusBootstrapPending), passes with itsrc/migration/__tests__/migrator.spec.tsrun — no changes to any existing test outcomeusesVitest = true(both rewritten to managed specs, detection converges) and a nested object in yarnresolutions(converges without crashing)packages/cliis clean