Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] aliased dependencies incorrectly de-dupe a transitive dependency #3154

Open
panva opened this issue Apr 27, 2021 · 7 comments
Open

[BUG] aliased dependencies incorrectly de-dupe a transitive dependency #3154

panva opened this issue Apr 27, 2021 · 7 comments
Labels
Bug thing that needs fixing config:aliases

Comments

@panva
Copy link

panva commented Apr 27, 2021

Current Behavior:

When installing a direct dependency using the alias syntax e.g. npm install jose@npm:jose-node-esm-runtime my other dependencies that depend on jose start requiring jose-node-esm-runtime instead.

Expected Behavior:

Aliased modules do not trigger de-dupe based on the alias value.

Steps To Reproduce:

mkdir poc && cd poc
npm init -y
npm install oidc-provider
node -e 'require("oidc-provider")' // works
npm install jose@npm:jose-node-esm-runtime
node -e 'require("oidc-provider")' // throws because the `jose` depended upon by oidc-provider is gone and was replaced by jose-node-esm-runtime
// the throw itself is not important, it's throwing because the oidc-provider > jose dependency is gone from node_modules

// further quirks
npm uninstall jose
node -e 'require("oidc-provider")' // still wrong because jose is still resolved to `jose-node-esm-runtime` in package-lock.json, eventho it is no longer to be found in package.json

Environment:

Node 14.16.0
npm 7.11.1, npm@6 as well

@panva panva added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Apr 27, 2021
@panva panva changed the title [BUG] aliased incorrectly de-dupes a transitive dependency [BUG] aliased dependencies incorrectly de-dupe a transitive dependency Apr 27, 2021
@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

Isn’t that the whole point of the aliasing?

@panva
Copy link
Author

panva commented Apr 27, 2021

https://docs.npmjs.com/cli/v7/commands/npm-install

Install a package under a custom alias. Allows multiple versions of a same-name package side-by-side, more convenient import names for packages with otherwise long ones, and using git forks replacements or forked npm packages as replacements. Aliasing works only on your project and does not rename packages in transitive dependencies.

Isn’t that the whole point of the aliasing?

To have a transitive dependency all of a sudden replaced by something else? I sure hope not and the docs do state anything about changing my dependencies' dependencies being affected.

I didn't dig deep enough but could be a vector for supply chain poisoning too.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

It could not be a vector for that because using an alias is an explicit choice by the user.

It’s definitely the point. If i, for example, install react experimental aliased as “react”, and it didnt replace “react” for transitive deps, my application would be broken. The feature isn’t for a convenient way to refer to something; it’s literally remapping one dependency name to another.

@panva
Copy link
Author

panva commented Apr 27, 2021

It could not be a vector for that because using an alias is an explicit choice by the user.

As I said, I didn't dig deep enough to check how e.g. a published package I would depend on using an alias to "react" affected the dependency tree.

It’s definitely the point. If i, for example, install react experimental aliased as “react”, and it didnt replace “react” for transitive deps, my application would be broken. The feature isn’t for a convenient way to refer to something; it’s literally remapping one dependency name to another.

From the way the docs are stated it definitely points to being a convenience thing. So that I don't have to change my code to refer to a forked package name but rather, i choose to alias. But the point is my code, my direct dependencies. It affecting transitive dependencies is not expected. The docs actually state explicitly that it does not rename packages in transitive dependencies, which kinda sounds like what is happening in the POC above.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

It renames whatever’s in the top level of node_modules, which may or may not be a transitive dependency. Perhaps the docs need improvement here.

@panva
Copy link
Author

panva commented Apr 27, 2021

more convenient import names for packages with otherwise long ones

Say that's the reason why I chose to use an alias. having it affect any transitive dependency is from my pov unexpected. The cli does not let me know where the alias is in effect in my current tree, and i can't possibly know that an alias will not conflict with a future transitive dependency.

What you're saying makes it sound more like overrides. It's either a bug or insufficient docs, I'm fine either way.

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

It is indeed very like overrides, but doesn’t provide the same guarantees.

@darcyclarke darcyclarke assigned darcyclarke and isaacs and unassigned darcyclarke Jun 11, 2021
@darcyclarke darcyclarke removed the Needs Triage needs review for next steps label Jun 11, 2021
@lukekarrys lukekarrys removed the Release 7.x work is associated with a specific npm 7 release label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing config:aliases
Projects
None yet
Development

No branches or pull requests

5 participants