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 install <dep>@version removes the dep from peerDeps if dep is in devDeps as well #1849

Closed
scamden opened this issue Sep 23, 2020 · 7 comments
Assignees
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release

Comments

@scamden
Copy link

scamden commented Sep 23, 2020

Current Behavior:

running npm install on a dependency that is in both dev and peer deps removes the dep from peer. (note: this even happens if installing with the --legacy-peer-deps flag)

Expected Behavior:

if anything the peer dep should be kept over the dev dep, as the peer dep is part of the package's outward facing contract. preferably both are kept while legacy-peer-deps is an option.

Steps To Reproduce:

  1. create a package.json with a dep in both dev and peer
  2. run npm install <depname>@latest in the package
  3. notice that the dep is no longer in peer deps

Environment:

@scamden scamden 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 Sep 23, 2020
@scamden scamden changed the title [BUG] npm install <dep>@version removes peer dep if dep is in devDeps [BUG] npm install <dep>@version removes the dep from peerDeps if dep is in devDeps as well Sep 23, 2020
@ljharb
Copy link
Contributor

ljharb commented Sep 23, 2020

Peer deps should almost always be also in devDeps, or also in runtime deps, so this seems like a big issue. Thanks for reporting it!

To confirm, this is with which npm version?

@aliatsis
Copy link

@ljharb we tested and reproduced with 7.0.0-beta.10, 7.0.0-beta.11, and 7.0.0-beta.12

@darcyclarke darcyclarke added beta and removed Needs Triage needs review for next steps labels Sep 25, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 16 milestone Sep 25, 2020
@isaacs
Copy link
Contributor

isaacs commented Sep 30, 2020

Hmm... so this is a pretty easy fix, to just note that it's in both places and save it back to both. However, I notice that the "pretty easy" fix means that the two fields will stay in sync, which could either be a surprising bug or a really wonderful feature.

So, for example, if your package.json is:

{
  "devDependencies": {
    "foo": "^1.3.5"
  },
  "peerDependencies": {
    "foo": "1.x"
  }
}

and then you do: npm i foo@latest, you might get:

{
  "devDependencies": {
    "foo": "^1.5.2"
  },
  "peerDependencies": {
    "foo": "^1.5.2"
  }
}

This can be a hazard if you want to ensure that you're testing with the latest and greatest version, but also want to have a broader peerDependencies range to avoid peer dep conflicts. Another view of this is that having these two things out of sync is almost always a bug, since it means you're not testing with the version that you're going to be loading at run-time.

In practice, we prioritize the peer dependency when loading edges, so the fact that we were prioritizing dev at save time is definitely a bug. Saving back to both seems like the right thing to do (and is trivial to do), with "saving to peer only" as a possible second place option. Saving to peer and dev differently will be somewhat tricky.

@ljharb
Copy link
Contributor

ljharb commented Sep 30, 2020

imo they should always indeed be in sync, so I find that to be a really wonderful feature.

That said, whatever's in peer deps should be the source of truth, since that's exposed as part of your API.

isaacs added a commit to npm/arborist that referenced this issue Sep 30, 2020
@isaacs
Copy link
Contributor

isaacs commented Sep 30, 2020

The peerDep is the source of truth (since it's loaded first, and we skip the creation of edges if we already have an edge by that name). When saving, they're both saved to the same value.

isaacs added a commit to npm/arborist that referenced this issue Sep 30, 2020
@scamden
Copy link
Author

scamden commented Sep 30, 2020

yep makes good sense to me. i've often wanted the peer dep to update when I install the latest :)

@ljharb
Copy link
Contributor

ljharb commented Sep 30, 2020

The challenge there is that for a peer dep, if the minimum version changes, it's a breaking change.

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

Successfully merging a pull request may close this issue.

5 participants