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] npm dedupe fails to dedupe newer version of dependency when older version of dependency is at root level in package-lock.json #5307

Closed
2 tasks done
dstaley opened this issue Aug 16, 2022 · 2 comments
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release

Comments

@dstaley
Copy link

dstaley commented Aug 16, 2022

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

When running npm dedupe on a package tree containing several instances of a package dependency at the same semver range, the package is not lifted to the root level of the package tree, and multiple versions of the package are installed.

I believe this is a result of npm installing an older version of the package at the root-level, preventing the newer shared version from being lifted to the root level. This hypothesis is supported by the fact that removing the package that relies on the older version of the dependency results in npm dedupe working correctly.

In the linked repo, @hashicorp/react-combobox depends on @reach/combobox, which depends on @reach/[email protected]. Since this package was the first installed, npm stored @reach/[email protected] at node_modules/@reach/auto-id, and persisted this to package-lock.json. When @hashicorp/react-text-input and @hashicorp/react-textarea-input were installed, npm was unable to dedupe @reach/[email protected] because the only available storage location was already taken by @reach/[email protected].

[email protected] C:\Users\DylanStaley\Desktop\auto-id
├─┬ @hashicorp/[email protected]
│ └─┬ @reach/[email protected]
│   └── @reach/[email protected]
├─┬ @hashicorp/[email protected]
│ └── @reach/[email protected]
└─┬ @hashicorp/[email protected]
  └── @reach/[email protected]

I'm fairly sure this is a result of npm using layout data from package-lock.json since installing without a lockfile results in correct deduplication. This leads me to believe that modern versions of npm have a dedupe algorithm that can solve for this specific case, but it instead relies on the layout information from package-lock.json, resulting in a package tree that's different from what you'd receive if you had run npm install without a lockfile.

Expected Behavior

Packages that depend on the same semver range of a specific package have their dependencies deduped even if the package-lock.json lockfile has a different version of the package at the root level.

In the provided repro, after npm dedupe I'd expect @reach/[email protected] to be moved into node_modules/@reach/combobox/node_modules/@reach/auto-id, and @reach/[email protected] to be moved from node_modules/@hashicorp/react-text-input/node_modules/@reach/auto-id to node_modules/@reach/auto-id.

Steps To Reproduce

  1. Clone this repo.
  2. Run npm install followed by npm dedupe.
  3. Confirm package @reach/auto-id at v0.17.0 is not deduped by running npm ls @reach/auto-id.

Environment

  • npm: 8.17.0
  • Node.js: v16.14.0
  • OS Name: Windows
  • System Model Name: Dell XPS 15
  • npm config:
; "user" config from C:\Users\DylanStaley\.npmrc

//registry.npmjs.org/:_authToken = (protected)

; node bin location = C:\Users\DylanStaley\AppData\Local\Volta\tools\image\node\16.14.0\node.exe
; cwd = C:\Users\DylanStaley\Desktop\auto-id
; HOME = C:\Users\DylanStaley
; Run `npm config ls -l` to show all defaults.
@dstaley dstaley added Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release labels Aug 16, 2022
@wraithgar
Copy link
Member

First off it's good to understand that dedupe is not the same concept as hoist. Dedupe can happen anywhere in the node_modules tree, it simply means more than one package sees it when using require. Hoisting means that a package is at the very top of your node_modules.

So yes, the presence of a hoisted package sometimes prevents the "optimal" deduping strategy (which in your case 0.17 would be hoisted and 0.16 would then be moved down under the only package that is requiring it). Dedupe does not guarantee this contract though. It finds what it can reasonably do, but doesn't rebuild the tree from scratch. There is already a way to rebuild the tree from scratch and that is to remove the package-lock. npm has never had the contract that every update will result in the same lockfile as a fresh install.

There is currently no mechanism to "force" npm to hoist a given dependency over another. I've seen some folks install the dependency they want hoisted, then uninstall it so that the right version is hoisted and stays there. We have even done this for the cli itself when version bumps leave us in this exact situation. Realistically though your best option if this is a big enough concern for you is to periodically rebuild your lockfile.

Whether or not this is a concern is up to you though. It's ok to have duped dependencies unless those dependencies are incorrectly assuming they will be singletons across multiple modules that require them, in which case you may need to use npm query to lint whether or not the dependency has been hoisted. There is an ongoing rfc discussion about what to do with packages that make this assumption.

TLDR: yep, this is how npm works.

@dstaley
Copy link
Author

dstaley commented Aug 16, 2022

Thank you for the explanation! 💪 You're dead on that this is one of those dependencies that assumes it'll only be installed once across the tree. I'll keep an eye on the RFC discussion, but in the meantime give the install+uninstall trick a go since that does sound like it'd fix this specific issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

No branches or pull requests

2 participants