[BUG] npm can install an invalid tree with transitive optional peer deps

[billyjanitsch]

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

Given the following packages:

{
  "name": "app",
  "version": "1.0.0",
  "dependencies": {
    "a": "^1.0.0",
    "b": "^1.0.0"
  }
}

{
  "name": "a",
  "version": "1.0.0",
  "peerDependencies": {
    "c": "^1.0.0"
  },
  "peerDependenciesMeta": {
    "c": {
      "optional": true
    }
  }
}

{
  "name": "b",
  "version": "1.0.0",
  "dependencies": {
    "c": "^2.0.0"
  }
}

{
  "name": "c"
  "version": "1.0.0"
}

{
  "name": "c"
  "version": "2.0.0"
}

Running npm install in app completes successfully, but results in the following invalid tree (npm ls):

[email protected]
├─┬ [email protected]
│ └── [email protected] invalid: "^1.0.0" from node_modules/a
└─┬ [email protected]
  └── [email protected] deduped invalid: "^1.0.0" from node_modules/a

Basically, b's version of c gets incorrectly hoisted, resulting in an incompatible peer dep for a.

npm ci also refuses to install the tree that npm install just generated:

npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.

Related to #4664. That issue was closed but there does seem to be an npm bug here.

Expected Behavior

I haven't thought through the details carefully, but it seems that b's dependency on c shouldn't be hoisted if it would result in an invalid optional peer dep.

Steps To Reproduce

This reproduces in npm 8.6.0 through 8.9.0.

1. Clone https://github.com/billyjanitsch/npm-optional-peer-deps-hoist
2. Note the two separate commits. The first commits the package.json, and the second commits the package-lock.json generated by npm install.
3. Run npm ci or npm ls; both exit with an error.

@pmmmwh/[email protected] optionally peer-depends on type-fest@>=0.17.0 <3.0.0. ow directly depends on type-fest@^0.11.0. The latter gets incorrectly hoisted.

Environment

• npm: 8.9.0
• Node.js: 16.15.0
• OS Name: macOS
• System Model Name: MacBook Pro
• npm config:

; n/a

[billyjanitsch]

(cc @lukekarrys, per your comment in #4664 (comment).)

[bnb]

I think I'm also hitting this on 8.9.0 and it's causing a Dockerfile build to fail, though it's not restricted to peerDeps - all my deps are failing to install with npm ci.

[bnb]

created a Gist of my package.json, package-lock.json, and log file: https://gist.github.com/bnb/f05ed6601c7997c0453d062ac7c23a64

I've nuked node_modules and run npm install and then npm ci --only=production again, get the same error output. I've also just run npm install without nuking node_modules and run npm ci --only=production and also get the same error output.

[billyjanitsch]

@bnb I don't think yours is the same issue. They share the same error message ("npm ci can only install packages...") but the failure state is different. In my case, npm install has produced an invalid tree/lockfile due to an interaction between peer deps and hoisting, and npm ci is (correctly) realizing that the tree is invalid and refuses to install.

I don't believe that your case is a bug, though the error message could be improved. You generally need to run npm install with the same args/config as npm ci in order for the latter's lockfile validity check to work correctly. On 8.9.0, starting from your package.json and package-lock.json, if I run npm install --only=production && npm ci --only=production, both installs complete without error. (If you can't reproduce this, I'm curious if there's anything in your config (npm get) that might be altering the lockfile generation.)

The error message should probably make clear that npm install needs to be run with the same config as npm ci, but it might be best to note that in a new issue since that's not a factor in this one.

[darcyclarke]

@billyjanitsch Thanks for the thorough report & repro! I was able to reproduce (on v8.5.5 & v8.9.0 -- so this is notably not related to v8.6.0 specifically).

Funny enough, I was able to run npm install a second time & that resolved the problem for me (so likely a race condition &/or missing condition in canPlaceDep when we're building the ideal tree from scratch -- ie. without a lockfile or node_modules).

How to unblock yourself right now:

• remove node_modules/package-lock.json
• run npm install
• run npm install (again)
• run npm ci or npm ls --all should now succeed (& the lockfile should be accurate)

The issue is as you explained it; the direct dep gets hoisted the first time we reify but it conflicts with the optional peer. Running install a second time (w/ a lockfile) will actually correctly nest the direct dep & hoist the peer (updating the lockfile in-turn & hopefully avoiding future problems).

We'll definitely look into where to solve this early next week.

[billyjanitsch]

Thanks @darcyclarke. I mistakenly assumed that this didn't occur on <8.6.0 because I was using the npm ci failure as a test, but ofc it won't fail prior to 8.6.0 because that's when the check was added, even if the tree is invalid in this way. Thanks for looking into it and suggesting a stopgap workaround. 🙂

[darcyclarke]

All good, we'll get a fix in soon.

[Alex69222]

@billyjanitsch Thanks for the thorough report & repro! I was able to reproduce (on v8.5.5 & v8.9.0 -- so this is notably not related to v8.6.0 specifically).

Funny enough, I was able to run npm install a second time & that resolved the problem for me (so likely a race condition &/or missing condition in canPlaceDep when we're building the ideal tree from scratch -- ie. without a lockfile or node_modules).

How to unblock yourself right now:

• remove node_modules/package-lock.json
• run npm install
• run npm install (again)
• run npm ci or npm ls --all should now succeed (& the lockfile should be accurate)

The issue is as you explained it; the direct dep gets hoisted the first time we reify but it conflicts with the optional peer. Running install a second time (w/ a lockfile) will actually correctly nest the direct dep & hoist the peer (updating the lockfile in-turn & hopefully avoiding future problems).

We'll definitely look into where to solve this early next week.

Thanks! That worked!

[nlf]

hi there folks, linked above is a pull request that addresses this problem. it was unfortunately not as simple to fix as we had hoped. the reason for the strange output is that we don't actually install optional peer dependencies at all today unless you have them declared as your own dependency too. since your example doesn't do this, we end up installing c@2 to node_modules/c and not even noticing that we could've placed c@1 there and nested c@2 to create a complete and correct package tree.

the fix is changing this behavior such that a missing optional peer will attempt to install. if it fails, we prune it from the tree. if it's in conflict with another type of node at the same location, the optional peer takes the lowest precedent and may become invalid.

[billyjanitsch]

@nlf IMO, that doesn't seem like the right way to fix this bug.

we don't actually install optional peer dependencies at all today unless you have them declared as your own dependency too

This is intentional, according to the RFC (npm/rfcs#224) and the implementation commit (npm/arborist@e13ba3b).

I don't understand why you'd want to change this behavior. The ecosystem has widely embraced optional peer dependencies specifically as a way to avoid auto-installation, and the RFC explicitly states this as a goal. The proposed change would leave npm without any way to specify a version constraint without auto-installing the associated package. I understand from the PR that you're planning to delay the change until the next major npm release, but why make that change at all?

[nlf]

this is a proposed fix, not a concrete solution. the problem as it stands is that we either place a node or we do not. if we do not place a node in the tree, there is absolutely no means today for us to ensure that another node doesn't end up in that location. this is a limitation of our current code base that we're aware of and have long term plans to address.

the second problem is the inconsistency in the meaning of "optional". as you pointed out there is an rfc suggesting that we not attempt to automatically install an optional peer dependency, but that rfc made no mention whatsoever regarding a regular optional dependency (i.e. one listed in optionalDependencies). this means that while we won't attempt to install an optional peer, we will attempt to install a regular optional dependency. this split in behavior causes confusion and a lot of subtle bugs in the inner workings of arborist.

personally, i'd like to see us not install any optional dependencies by default. if we can make the argument that an optional peer should only be installed when directly requested, then we should be able to make the same argument about any other optional dependency.

between that and a very significant refactoring to consider certain tree locations to be off limits we can get pretty far, but there will always be corner cases where we end up doing something the user may not expect.

as an example, let's say i have a project defined like so:

{
  "name": "my-project",
  "version": "1.0.0",
  "dependencies": {
    "a": "^1.0.0",
    "b": "^1.0.0"
  }
}

[email protected] has a package.json like this:

{
  "name": "a",
  "version": "1.0.0",
  "peerDependencies": {
    "c": "^1.0.0"
  },
  "peerDependenciesMeta": {
    "c": {
      "optional": true
    },
  }
}

while [email protected] looks like:

{
  "name": "b",
  "version": "1.0.0",
  "peerDependencies": {
    "c": "^2.0.0"
  },
  "peerDependenciesMeta": {
    "c": {
      "optional": true
    }
  }
}

if a user runs npm i with the dependencies listed above with today's behavior, we don't install either version of c. however, if the user adds a direct dependency of "c": "^1.0.0", then [email protected] will be placed at node_modules/c because that's the only location it can be and now [email protected]'s optional peer dependency on c@^2.0.0 is invalid.

the invalid dependency is an optional one so throwing an error doesn't feel right because you didn't ask for that optional dependency, however it is physically impossible for us to install the 3 requested dependencies without rendering the 4th unrequested one invalid. so what's the right behavior? force the user to add an override saying that [email protected] is fine for b? what if it's not fine for some reason and this causes a runtime error? is that better or worse than npm leaving a known invalid node in place?

if it's not clear, what i'm getting at here is that there are an overwhelming number of corner cases out there and we cannot solve for all of them, especially when it comes to peer dependencies. even if we went back to not installing any peer dependencies at all, there's still the problem that the definitions for peers existing in your tree means you're still likely to end up with an invalid node somewhere.

i hear what you're saying, and i agree that reversing a decision made in a past rfc can be a bit alarming (though i would also note that there is absolutely nothing saying we can't or won't reverse a past decision in the future). i just want to assure you that it's not something we take lightly and will not be making a change like this without planning and clear reasoning. my ultimate end goal is to start developing a formal specification for how npm decides what modules to install and where to install them. it's going to be a long road, but we will get there.

[ljharb]

I think the difference is that an optional dependency is something the end user shouldn't have to know about or specify - so it makes sense to install it by default. However, an optional peer dep IS something the end user has to know about and specify - so it makes sense not to install it by default.

In other words, even many breaking changes from now, I don't think these two optional categories should be "consistent" with each other in terms of auto-installation.

[nlf]

so a user is expected to know about an optional peer but not about an optional prod dependency? that doesn't really make sense to me.

i can concede that the two don't need to behave the same way, but if that's the case then we need to refactor arborist to stop referring to them both the same way. the optional property on a node will be set for both an optional prod dep as well as an optional peer, but if the meaning is different then they should very much not set the same property

[ljharb]

Yes, a peer (optional or not) is a part of the API that a user always has to know about - adding one, or raising the version threshold on one, is always a breaking change.

I have no opinion about the arborist representation.

[mutantcornholio]

Hi all!
I've got an issue when failing to install regular optional dependency leads to hoisting of a different version of the dependency. Is this the same issue?

npm init
# vite declares fsevents@~2.3.3 in optionalDependencies
# remixd depends on [email protected], which depends on fsevents@^1.2.7
npm install --save vite @remix-project/remixd
# If [email protected] fails to install, we'll get fsevents@^1.2.7 on top-level node_modules

more info: vitejs/vite#18728