Skip to content

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

Closed
#8981
Closed
[BUG] npm can install an invalid tree with transitive optional peer deps#4859
#8981
Labels
Bugthing that needs fixingPriority 1high priority issueRelease 8.xwork is associated with a specific npm 8 release

Description

@billyjanitsch
billyjanitsch
opened on May 4, 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

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):

app@1.0.0
├─┬ a@1.0.0
│ └── c@2.0.0 invalid: "^1.0.0" from node_modules/a
└─┬ b@1.0.0
  └── c@2.0.0 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/react-refresh-webpack-plugin@0.5.5 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

Activity

  1. billyjanitsch
    added
    Bugthing that needs fixing
    Needs Triageneeds review for next steps
    Release 8.xwork is associated with a specific npm 8 release
    on May 4, 2022

  2. billyjanitsch commented on May 4, 2022

    @billyjanitsch
    billyjanitsch
    on May 4, 2022
    Author

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

  3. bnb commented on May 6, 2022

    @bnb
    bnb
    on May 6, 2022
    Contributor

    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.

  4. bnb commented on May 6, 2022

    @bnb
    bnb
    on May 6, 2022
    Contributor

    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.

  5. billyjanitsch commented on May 6, 2022

    @billyjanitsch
    billyjanitsch
    on May 6, 2022
    Author

    @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.

  6. darcyclarke commented on May 6, 2022

    @darcyclarke
    darcyclarke
    on May 6, 2022
    Contributor

    @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.

  7. billyjanitsch commented on May 6, 2022

    @billyjanitsch
    billyjanitsch
    on May 6, 2022
    Author

    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. 🙂

  8. billyjanitsch
    changed the title [-][BUG] >=8.6.0 can install an invalid tree with transitive optional peer deps[/-] [+][BUG] npm can install an invalid tree with transitive optional peer deps[/+] on May 6, 2022

  9. darcyclarke commented on May 6, 2022

    @darcyclarke
    darcyclarke
    on May 6, 2022
    Contributor

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

  10. darcyclarke
    added
    Priority 1high priority issue
    Priority 0will get attention right away
    and removed
    Needs Triageneeds review for next steps
    Priority 1high priority issue
    on May 6, 2022

  11. 9 remaining items

  12. nlf commented on Aug 26, 2022

    @nlf
    nlf
    on Aug 26, 2022
    Contributor

    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"
  }
}

    a@1.0.0 has a package.json like this:

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

    while b@1.0.0 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 c@1.0.0 will be placed at node_modules/c because that's the only location it can be and now b@1.0.0'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 c@1.0.0 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.

  13. ljharb commented on Aug 26, 2022

    @ljharb
    ljharb
    on Aug 26, 2022
    Contributor

    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.

  14. nlf commented on Aug 29, 2022

    @nlf
    nlf
    on Aug 29, 2022
    Contributor

    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

  15. ljharb commented on Aug 29, 2022

    @ljharb
    ljharb
    on Aug 29, 2022
    Contributor

    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.

  16. jaylinski
    added 2 commits that reference this issue on Oct 16, 2022

    Test on Node 18 and drop EOL Node 10

    2c88d6d

    Test on Node 18 and drop EOL Node 10

    785a63e
  17. wraithgar
    added
    Priority 1high priority issue
    and removed
    Priority 0will get attention right away
    on Dec 12, 2022
  18. wraithgar
    unassigned
    nlf
    on Dec 12, 2022

  19. mutantcornholio commented on Nov 22, 2024

    @mutantcornholio
    mutantcornholio
    on Nov 22, 2024

    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 chokidar@2.1.8, which depends on fsevents@^1.2.7
npm install --save vite @remix-project/remixd
# If fsevents@2.3.3 fails to install, we'll get fsevents@^1.2.7 on top-level node_modules

    more info: vitejs/vite#18728

  20. restjohn commented on Mar 7, 2025

    @restjohn
    restjohn
    on Mar 7, 2025

    I have a hunch this is related to a bug I reported as well: #7622.

  21. Saibamen commented on Feb 19, 2026

    @Saibamen
    Saibamen
    on Feb 19, 2026
    Contributor

    Tested with #8981:

    npm ci: OK (both from 11.10.0 and PR #8981 (448db52))

    npm ls (the same output for both versions):

    npm-optional-peer-deps-hoist@0.0.0 C:\DEV\npm_test\4859
+-- @pmmmwh/react-refresh-webpack-plugin@0.5.17
+-- ow@0.17.0
`-- type-fest@4.41.0 extraneous

    git diff between package-lock.json files from the first npm install - NPM 11.10.0 vs NPM from PR #8981 (448db52):

    diff --git a/package-lock.json b/package-lock.json
index d1d64c0..42c4058 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -828,6 +828,18 @@
         "url": "https://github.com/sponsors/sindresorhus"
       }
     },
+    "node_modules/ow/node_modules/type-fest": {
+      "version": "0.11.0",
+      "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-0.11.0.tgz",
+      "integrity": "sha512-OdjXJxnCN1AvyLSzeKIgXTXxV+99ZuXl3Hpo9XpJAv9MBcHrrJOQ5kV7ypXOuQie+AmWG25hLbiKdwYTifzcfQ==",
+      "license": "(MIT OR CC0-1.0)",
+      "engines": {
+        "node": ">=8"
+      },
+      "funding": {
+        "url": "https://github.com/sponsors/sindresorhus"
+      }
+    },
     "node_modules/picocolors": {
       "version": "1.1.1",
       "resolved": "https://registry.npmjs.org/picocolors/-/picocolors-1.1.1.tgz",
@@ -1035,12 +1047,13 @@
       }
     },
     "node_modules/type-fest": {
-      "version": "0.11.0",
-      "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-0.11.0.tgz",
-      "integrity": "sha512-OdjXJxnCN1AvyLSzeKIgXTXxV+99ZuXl3Hpo9XpJAv9MBcHrrJOQ5kV7ypXOuQie+AmWG25hLbiKdwYTifzcfQ==",
+      "version": "4.41.0",
+      "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-4.41.0.tgz",
+      "integrity": "sha512-TeTSQ6H5YHvpqVwBRcnLDCBnDOHWYu7IvGbHT6N8AOymcr9PJGjc1GTtiWZTYg0NCgYwvnYWEkVChQAr9bjfwA==",
+      "extraneous": true,
       "license": "(MIT OR CC0-1.0)",
       "engines": {
-        "node": ">=8"
+        "node": ">=16"
       },
       "funding": {
         "url": "https://github.com/sponsors/sindresorhus"
  22. owlstronaut
    closed this as completedon Feb 19, 2026

  23. Saibamen commented on Feb 19, 2026

    @Saibamen
    Saibamen
    on Feb 19, 2026
    Contributor

    Fixed in NPM 11.10.1. Please update your NPM:
    npm install -g npm@latest

  24. zeekay
    added a commit that references this issue on Sep 9, 2026

    Test on Node 18 and drop EOL Node 10

    b8035c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Bugthing that needs fixingPriority 1high priority issueRelease 8.xwork is associated with a specific npm 8 release

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions