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

[RRFC] Prefer peerDependencies over regular dependencies, when both specified together #324

Closed
larixer opened this issue Feb 16, 2021 · 15 comments

Comments

@larixer
Copy link

larixer commented Feb 16, 2021

Motivation ("The Why")

The semantic is not well defined yet for the case when the same package is specified both as a peer dependency and a regular dependency, at least I have not been able to find a clear definition what should happen in this situation. It would be great if peer dependency had a priority over regular dependency in this case with fallback to a regular dependency if parent had not satisfied peer dependency.

Example

package.json:

{
  "name": "example",
  "version": "1.0.0",
  "dependencies": {
    "bar": "file:./bar",
    "left-pad": "1.1.0"
  }
}

bar/package.json:

{
  "name": "bar",
  "version": "1.0.0",
  "dependencies": {
    "left-pad": "^1.3.0"
  },
  "peerDependencies": {
    "left-pad": ">= 1.0.0"
  }
}

How

Current Behaviour

Currently npm ignores peerDependencies definition and treats foo as a regular dependency. This results in node_modules/left-pad@1.1.0 and bar/node_modules/left-pad@1.3.0 to be installed.

Desired Behaviour

It would be great if npm gave priority to peerDependencies in the situation when the dependency is both a peer and a regular dependency.

This would have resulted only in one version of node_moduules/left-pad@1.1.0 being installed and used both by example and by bar packages in the example above.

It would also lead to a semantically correct way to define singleton packages in the whole dependency graph of a project, by listing singleton packages as both dependencies and peerDependencies without a need from a user to make a decision how to satisfy these peerDependencies. It will also gave the user a mechanism to override the version of the singleton for the whole dependency graph by providing a version for such a dependency in the root package.json.

References

  • n/a
@ljharb
Copy link
Contributor

ljharb commented Feb 16, 2021

Shipping something as both a dep and a peer dep is a common technique pre-npm-7 to automatically install a dep, but still fail npm ls if the consumer isn’t complying with the peer dep. this seems like an important regression to avoid.

@larixer
Copy link
Author

larixer commented Feb 17, 2021

@ljharb Just to be sure the idea of the RRFC is clear. When shipping something as dep and a peer dep, the peer dep part is currently ignored, both by npm@7 and by npm@6. The idea is for npm to give priority to a peer dep part (when parent did satisfied peer dep) and pick left-pad@1.1.0 from the parent, which will result only in one left-pad version installed in this example project.

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2021

While that does make sense; i also think anything that appears in peer deps should also have an identical range when it also appears in deps or dev deps, so maybe that’s why i haven’t ran into this.

altho the more i think about it, i think that would be incorrect. By specifying the higher range in deps, the package is explicitly saying that it requires that to function, so it seems like npm is doing the right thing here.

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Feb 17, 2021
@larixer
Copy link
Author

larixer commented Feb 17, 2021

Yes, npm certainly not doing anything wrong. But perhaps npm could provide more value to the users in this case, if the semantics were treated different, which would not be wrong either.

Consider an example of Next.js framework. The Next.js authors want their users to not bother with installing webpack, and the Next.js authors declare webpack a regular dependency of next.

Users at some point might want to use some webpack plugin, for example let it be val-loader, val-loader plugin peer depends on webpack, and pretty much any webpack plugin peer depends on webpack. User cannot just add val-loader to their next project alone, they must also satisfy peer dependency of this plugin on webpack, so the user must add webpack as well to root package.json to satisfy this peer dependency. And after that the next-project of the user might end up with two different versions of webpack, one picked up from root package.json and another one picked up from next itself. And we know that having two different versions of webpack in a project is a troublesome situation, the intent is always to have only one version of webpack per project.

If next had shipped webpack as both dependency and a peer dependency and npm gave priority to peer dependency part first, then user-provided webpack version would have been picked by both next and val-loader and it would have been the only webpack version in the project.

@wesleytodd
Copy link

This would be a pretty big breaking change, right? There are a ton of edges with peer deps, and I think @ljharb's approach of the peer range matching is the only good one. Could we do a middle ground between this breaking change and instead start warning when a package publishes with miss-matched peer/regular dep ranges?

This would help authors of libs like Next.js to move toward correcting their package.json's while giving time for the new behaviors of npm@7 to settle in.

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2021

What I think would make sense, and be helpful, is warning when the top-level package.json had a dep in both peerDeps and deps or devDeps, and the range wasn't identical.

@larixer
Copy link
Author

larixer commented Feb 17, 2021

@ljharb I don't understand why the range is not identical though
^1.3.0 matches >= 1.0.0, no?

@larixer
Copy link
Author

larixer commented Feb 17, 2021

@darcyclarke Can I join todays meeting, that will start in a minute? Zoom asks for a password when I follow the link

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2021

@larixer it satisfies it, but it does not match it, since v1.2.0 would be allowed by the latter as well.

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2021

npm v7 can only have a single "edge" going from one node in the graph to another. That edge has a specific type and spec, but never more than one of these. When creating edges from the various types of deps listed in package.json, we do not create an edge if one already exists, so they're order dependent at load time.

We prefer dev over prod, because presumably that's the one you want to test against, and we don't load dev edges when it's not the project being developed anyway.

Preferring the peer edge over prod seems reasonable to me, since the semantics are more restrictive.

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2021

Easily fixed:

diff --git a/lib/node.js b/lib/node.js
index 9a6b86e..c37cab3 100644
--- a/lib/node.js
+++ b/lib/node.js
@@ -731,7 +731,6 @@ class Node {
     // Note the subtle breaking change from v6: it is no longer possible
     // to have a different spec for a devDep than production dep.
     this[_loadDepType](this.package.optionalDependencies, 'optional')
-    this[_loadDepType](this.package.dependencies, 'prod')
 
     // Linked targets that are disconnected from the tree are tops,
     // but don't have a 'path' field, only a 'realpath', because we
@@ -755,6 +754,7 @@ class Node {
       this[_loadDepType](peerDependencies, 'peer')
       this[_loadDepType](peerOptional, 'peerOptional')
     }
+    this[_loadDepType](this.package.dependencies, 'prod')
   }
 
   [_loadDepType] (obj, type) {
diff --git a/test/node.js b/test/node.js
index 69edab7..fcafbdb 100644
--- a/test/node.js
+++ b/test/node.js
@@ -2181,3 +2181,17 @@ t.test('globaTop set for children of global link root target', async t => {
   })
   t.equal(gtop.globalTop, true)
 })
+
+t.test('prefer peer over prod', async t => {
+  const n = new Node({
+    pkg: {
+      dependencies: {
+        foo: '1.x',
+      },
+      peerDependencies: {
+        foo: '1.x',
+      },
+    },
+  })
+  t.equal(n.edgesOut.get('foo').type, 'peer', 'prefer peer over prod dep')
+})

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2021

Note: also update npm help package.json with the logic.

@larixer
Copy link
Author

larixer commented Feb 17, 2021

@isaacs This change will make prefer peer dep over prod dep always. The idea of RRFC is prefer peer dep over prod dep if peer dep was provided, otherwise fallback to regular dep, does this make sense?

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2021

Yes, if the peer dep is not provided, then there will be no existing edge, so these are equivalent.

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2021

One issue to consider: if I have both peer and prod installed, and install an update, then npm 7 will only save to peerDependenices, and remove it from prod dependencies, so a bit more work is required for this patch to preserve/update it in both places, if we decide to go that route.

isaacs added a commit to npm/arborist that referenced this issue Feb 17, 2021
Also, preserve the duplication if we update or modify the package.json
file.

Fix: npm/rfcs#324
isaacs added a commit to npm/arborist that referenced this issue Feb 17, 2021
Also, preserve the duplication if we update or modify the package.json
file.

Fix: npm/rfcs#324
isaacs added a commit to npm/cli that referenced this issue Feb 18, 2021
* [#1875](#1875)
  [npm/arborist#230](npm/arborist#230) Set default
  advisory `severity`/`vulnerable_range` when missing from audit endpoint
  data ([@isaacs](https://github.com/isaacs))
* [npm/arborist#231](npm/arborist#231) skip
  optional deps with mismatched platform or engine
  ([@nlf](https://github.com/nlf))
* [#2251](#2251) Unpack shrinkwrapped deps
  not already unpacked ([@isaacs](https://github.com/isaacs),
  [@nlf](https://github.com/nlf))
* [#2714](#2714) Do not write package.json
  if nothing changed ([@isaacs](https://github.com/isaacs))
* [npm/rfcs#324](npm/rfcs#324) Prefer peer over
  prod dep, if both specified ([@isaacs](https://github.com/isaacs))
* [npm/arborist#236](npm/arborist#236) Fix
  additional peerOptional conflict cases
  ([@isaacs](https://github.com/isaacs))
@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants