npm / cli

the package manager for JavaScript
https://docs.npmjs.com/cli/
Other
8.46k stars 3.15k forks source link

[BUG] Updating a dependency in a workspace with `overrides` duplicates the package #6979

Open thislooksfun opened 11 months ago

thislooksfun commented 11 months ago

Is there an existing issue for this?

This issue exists in the latest npm version

Current Behavior

Updating a dependency should update the installed package in-place, not cause duplication.

Expected Behavior

The package is duplicated, and the old version is still present.

Steps To Reproduce

  1. Make a new project
  2. Enable workspaces, and make a workspace package (I'll refer to it as foo)
  3. Add a dependency to foo, and run npm install
  4. The dependency will be hoisted and installed at <root>/node_modules/<package>
  5. Update the dependency (either manually by editing the package.json file and running npm install, running npm install <package>@version from inside foo, or running npm install <package>@version -w<path/to/>foo`

At this point the new version of the dependency will be installed to <root>/<path/to/>foo/node_modules/<package>, and the old version will still be present at <root>/node_modules/<package>.

Environment

thislooksfun commented 11 months ago

Additional info:

wraithgar commented 11 months ago

Cannot reproduce with the steps given. I suspect what's happening here is that the old version is valid for some other dependency in the tree, but the new version isn't, so the new version won't hoist.

~/D/n/s/a $ npm init -y; npm init -y -w workspace-a
~/D/n/s/a $ npm i abbrev@1.1.0 -w workspace-a
~/D/n/s/a $ npm i abbrev@1.1.1 -w workspace-a
~/D/n/s/a $ npm ls
a@1.0.0 /Users/wraithgar/Development/npm/scratch/a
└─┬ workspace-a@1.0.0 -> ./workspace-a
  └── abbrev@1.1.1

~/D/n/s/a $ ls node_modules/
abbrev/      workspace-a@
~/D/n/s/a $ ls node_modules/workspace-a/
package.json
thislooksfun commented 11 months ago

Ah. It seems the bug is more subtle than I first thought. It only happens if the root package.json has a non-empty overrides definition.

Here is a script that reproduces the issue:

npm init -y
npm init -y -w workspace-a

# Add an "overrides" section to the root package.json
pkg=$(jq '.overrides |= {"ava": "5.3.1"}' package.json)
echo "$pkg" > package.json

npm i --save-exact abbrev@1.1.0 -w workspace-a
npm i --save-exact abbrev@1.1.1 -w workspace-a

Note that the overrides do not need to interact with the affected packages at all to cause the bug, merely being there is enough.

thislooksfun commented 11 months ago

To be clear, the above script will "unhoist" abbrev (move it down into the package), but it will not duplicate it. If you also run npm i --save-exact nopt@6.0.0 -w workspace-a before updating to abbrev@1.1.1 then it will be duplicated. Performing the same series of actions with out an overrides object will correctly update <root>/node_modules/abbrev in-place.

Examples

Update abbrev in-place (standalone) #### Script: ```sh npm init -y npm init -y -w workspace-a npm i --save-exact abbrev@1.1.0 -w workspace-a npm i --save-exact abbrev@1.1.1 -w workspace-a ``` #### Result: ```json { "name": "npm-repro", "version": "1.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "npm-repro", "version": "1.0.0", "license": "ISC", "workspaces": [ "workspace-a" ] }, "node_modules/abbrev": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==" }, "node_modules/workspace-a": { "resolved": "workspace-a", "link": true }, "workspace-a": { "version": "1.0.0", "license": "ISC", "dependencies": { "abbrev": "1.1.1" } } } } ```
Update abbrev in-place (external dependency) #### Script: ```sh npm init -y npm init -y -w workspace-a npm i --save-exact abbrev@1.1.0 -w workspace-a # noopt@6.0.0 depends on abbrev@^1.0.0 npm i --save-exact nopt@6.0.0 -w workspace-a npm i --save-exact abbrev@1.1.1 -w workspace-a ``` #### Result: ```json { "name": "npm-repro", "version": "1.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "npm-repro", "version": "1.0.0", "license": "ISC", "workspaces": [ "workspace-a" ] }, "node_modules/abbrev": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==" }, "node_modules/nopt": { "version": "6.0.0", "resolved": "https://registry.npmjs.org/nopt/-/nopt-6.0.0.tgz", "integrity": "sha512-ZwLpbTgdhuZUnZzjd7nb1ZV+4DoiC6/sfiVKok72ym/4Tlf+DFdlHYmT2JPmcNNWV6Pi3SDf1kT+A4r9RTuT9g==", "dependencies": { "abbrev": "^1.0.0" }, "bin": { "nopt": "bin/nopt.js" }, "engines": { "node": "^12.13.0 || ^14.15.0 || >=16.0.0" } }, "node_modules/workspace-a": { "resolved": "workspace-a", "link": true }, "workspace-a": { "version": "1.0.0", "license": "ISC", "dependencies": { "abbrev": "1.1.1", "nopt": "6.0.0" } } } } ```
Move abbrev into package #### Script: ```sh npm init -y npm init -y -w workspace-a # Add an "overrides" section to the root package.json pkg=$(jq '.overrides |= {"ava": "5.3.1"}' package.json) echo "$pkg" > package.json npm i --save-exact abbrev@1.1.0 -w workspace-a npm i --save-exact abbrev@1.1.1 -w workspace-a ``` #### Result: ```json { "name": "npm-repro", "version": "1.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "npm-repro", "version": "1.0.0", "license": "ISC", "workspaces": [ "workspace-a" ] }, "node_modules/workspace-a": { "resolved": "workspace-a", "link": true }, "workspace-a": { "version": "1.0.0", "license": "ISC", "dependencies": { "abbrev": "1.1.1" } }, "workspace-a/node_modules/abbrev": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==" } } } ```
Duplicate abbrev #### Script: ```sh npm init -y npm init -y -w workspace-a # Add an "overrides" section to the root package.json pkg=$(jq '.overrides |= {"ava": "5.3.1"}' package.json) echo "$pkg" > package.json npm i --save-exact abbrev@1.1.0 -w workspace-a npm i --save-exact nopt@6.0.0 -w workspace-a npm i --save-exact abbrev@1.1.1 -w workspace-a ``` #### Result: ```json { "name": "npm-repro", "version": "1.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "npm-repro", "version": "1.0.0", "license": "ISC", "workspaces": [ "workspace-a" ] }, "node_modules/abbrev": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.0.tgz", "integrity": "sha512-c92Vmq5hfBgXyoUaHqF8P5+7THGjvxAlB64tm3PiFSAcDww34ndmrlSOd3AUaBZoutDwX0dHz9nUUFoD1jEw0Q==" }, "node_modules/nopt": { "version": "6.0.0", "resolved": "https://registry.npmjs.org/nopt/-/nopt-6.0.0.tgz", "integrity": "sha512-ZwLpbTgdhuZUnZzjd7nb1ZV+4DoiC6/sfiVKok72ym/4Tlf+DFdlHYmT2JPmcNNWV6Pi3SDf1kT+A4r9RTuT9g==", "dependencies": { "abbrev": "^1.0.0" }, "bin": { "nopt": "bin/nopt.js" }, "engines": { "node": "^12.13.0 || ^14.15.0 || >=16.0.0" } }, "node_modules/workspace-a": { "resolved": "workspace-a", "link": true }, "workspace-a": { "version": "1.0.0", "license": "ISC", "dependencies": { "abbrev": "1.1.1", "nopt": "6.0.0" } }, "workspace-a/node_modules/abbrev": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==" } } } ```

Note: These scripts use the jq cli tool for some minor json editing, but those steps can be done by hand.

wraithgar commented 11 months ago

I still think this isn't a bug, just an inefficiency. The tree is still valid, and if you run npm install npm then builds the correct tree.

This is in the grey area of npm not trying to calculate the most efficient tree on every operation. There is no need, and it is ultimately an impossible task in some cases. It builds a correct tree.

The fact that the presence of overrides changes the behavior is definitely odd, and may be worth looking into if anyone has the time. This isn't something the npm team is going to prioritize, and I would be surprised if anyone in the community wanted to pick this up. You're welcome to dig into it if you'd like. I'll reopen this as Priority 2 and we'll see if anyone wants to look into it.

thislooksfun commented 8 months ago

I still think this is a bug. It's not uncommon for large applications to need to reach into dependencies of dependencies, and when there are multiple versions of that subdependency installed things can go weird; instanceof checks start failing (especially common when checking error types), Symbols don't line up, and code just generally breaks in weird ways. Yes there are typically workarounds for those cases, but it's still something that should just work out of the box.

ljharb commented 8 months ago

The only way to guarantee one version of a dep is if everything that needs it declares it as a peer dependency - thus, react, babel, webpack, eslint, etc, because it's only when identity matters that you'd need to ensure one copy of a dep.

What dep do you need unduplicated but that isn't commonly declared as a peer dep?

thislooksfun commented 8 months ago

I filed this issue after running into a problem with having multiple versions of the bson package installed in a monorepo (it's required by mongoose and has a Symbol that doesn't use Symbol.for, which was breaking comparisons), but unfortunately I don't remember exactly how I got into that situation and I'm having trouble recreating it from scratch. If/when I run into it again I'll update this issue.

ljharb commented 8 months ago

in that case, that's a bug in either bson or mongoose - iow, either it needs to be a peer dep, or it needs to use a global symbol.