fix(link-bins): apply bin ownership overrides in conflict resolution#10975
Conversation
BIN_OWNER_OVERRIDES was only used in checkGlobalBinConflicts for global installs. This change applies the same ownership rules in compareCommandsInConflict so that conflict resolution is consistent between global conflict checking and actual bin linking. This ensures packages like npm get priority for bins like npx even in non-global installs. Closes pnpm#10850
There was a problem hiding this comment.
Pull request overview
This PR aligns @pnpm/link-bins bin-conflict resolution with global install conflict logic by recognizing “bin ownership overrides” (e.g., treating npx as owned by npm) so the intended owner package wins conflicts consistently across install modes.
Changes:
- Added
BIN_OWNER_OVERRIDES+pkgOwnsBin()tolink-binsand used them incompareCommandsInConflict(). - Updated conflict resolution ordering to prioritize override owners before the existing
ownName/name/version tie-breakers. - Added a test intended to verify
npmwins thenpxbin conflict.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg-manager/link-bins/src/index.ts | Adds ownership-override awareness to bin conflict resolution. |
| pkg-manager/link-bins/test/index.ts | Adds a regression test for npx being owned by npm. |
| .changeset/apply-bin-owner-overrides-link-bins.md | Declares a minor release for the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
sounds good but don't duplicate code. |
Move shared logic to avoid code duplication between link-bins and checkGlobalBinConflicts.
|
I've refactored the code to extract |
| if (a.ownName && !b.ownName) return 1 | ||
| if (!a.ownName && b.ownName) return -1 |
There was a problem hiding this comment.
shouldn't this now just be removed? this check is already handled by pkgOwnsBin. The ownName field can probably be removed to.
Or alternatively, assign ownName the value returned by pkgOwnsBin. That way it will be cached.
There was a problem hiding this comment.
Done! Removed the redundant ownName field and checks since pkgOwnsBin already covers that case. Skipped caching as the performance gain would be negligible.
pkgOwnsBin already handles the binName === pkgName case, making the ownName field and its associated checks redundant.
Package renames on main: - @pnpm/package-bins -> @pnpm/bins.resolver - @pnpm/link-bins -> @pnpm/bins.linker - pkg-manager/link-bins -> bins/linker - pkg-manager/package-bins -> bins/resolver Updated imports, changeset package names, and moved test fixture to new location.
Added BIN_OWNER_OVERRIDES and pkgOwnsBin to @pnpm/bins.resolver for improved conflict resolution in bin linking.
Move fixture packages to the directory root instead of nesting them inside node_modules, avoiding committing node_modules to the repo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Congrats on merging your first pull request! 🎉🎉🎉 |
Problem
checkGlobalBinConflictshas aBIN_OWNER_OVERRIDESmap that recognizes certain bins as logically owned by a specific package even when the bin name doesn't match the package name (e.g.,npxis owned bynpm). This is used during global installs to decide whether a new package is allowed to override an existing bin.However, the link-bins conflict resolution (
compareCommandsInConflictinpkg-manager/link-bins/src/index.ts) only checksownName(cmd.name === manifest.name). It has no awareness of these ownership overrides. This means that in a non-global install, if multiple packages provide the same bin (e.g.,npx), the override owner (npm) doesn't get priority — the conflict falls through to alphabetical package name comparison.Solution
Apply the same ownership override rules in link-bins'
compareCommandsInConflictso that conflict resolution is consistent between global conflict checking and actual bin linking.Changes
BIN_OWNER_OVERRIDESconstant tolink-bins/src/index.ts(same as incheckGlobalBinConflicts)pkgOwnsBin()functioncompareCommandsInConflict()to check ownership overrides firstTest
Added test to verify
npmgets priority fornpxbin when multiple packages provide it.Closes #10850