Feature support NPM package aliases #45
Conversation
|
@Domantas-wix thanks for your contribution! I ran into this issue as well. @mgol, would love to see this merged =D |
Co-authored-by: Michał Gołębiowski-Owczarek <[email protected]>
| const supportsPackageAliases = packageManager => | ||
| packageManager === 'npm' || packageManager === 'yarn'; |
There was a problem hiding this comment.
Do we need this check? I imagine some other package managers like pnpm may also support aliases. We might want to exclude Bower here as that's completely different but in the rest I wouldn't assume lack of support.
There was a problem hiding this comment.
@mgol Yes, keeping a whitelist is secure, keeping a blacklist will eventually cause issues. All it takes is one package manager that interprets the colon differently to have huge discrepancies rendering the package unusable. Either way, when support for these other package managers is added code like this can be changed.
There was a problem hiding this comment.
By the way, not having sufficient checks like this is what prompted this PR in the first place, you take aliased version structure and pretend to know what it is, causing incorrect behaviour. Care should be taken to keep publicly used code forwards compatible
There was a problem hiding this comment.
pnpm already supports package aliases:
https://pnpm.js.org/en/aliases
and the syntax looks the same as the Yarn/npm one. It looks like the Yarn-originated syntax has become a de-facto standard that all package managers will soon follow. It's not likely that other package managers will implement it differently IMO.
Same changes as this other PR, except with the package manager check removed as requested in the PR comments. mgol#45
|
I was about to create a new PR addressing the feedback on this one to see if we could get this merged and released but realized @whoisbenli already attempted that? @mgol any idea why that didn't work out? |
|
@jcsmorais Hey! I had a remark about an allowlist approach excluding alternative package manager like pnpm plus the fact the Yarn-pioneered syntax for aliases became a de-facto standard so it makes sense to allow it by default and mostly just exclude from bower. If you could make a PR with commits from this one followed by the requested fixes, I'd be OK with merging this. |
|
Addressed feedback in #50 🙂 |
NPM has a poorly documented aliasing feature allowing user to install multiple instances of the same package under different names.
Proposal: https://github.com/npm/rfcs/blob/latest/implemented/0001-package-aliases.md
Implementation: npm/cli#3
Add support for these aliases, checking the installed version and if dependency matches by name.