-
Notifications
You must be signed in to change notification settings - Fork 239
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
Comments
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. |
@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 |
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. |
Yes, Consider an example of Users at some point might want to use some If |
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 |
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. |
@ljharb I don't understand why the range is not identical though |
@darcyclarke Can I join todays meeting, that will start in a minute? Zoom asks for a password when I follow the link |
@larixer it satisfies it, but it does not match it, since v1.2.0 would be allowed by the latter as well. |
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. |
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')
+})
|
Note: also update |
@isaacs This change will make |
Yes, if the peer dep is not provided, then there will be no existing edge, so these are equivalent. |
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. |
Also, preserve the duplication if we update or modify the package.json file. Fix: npm/rfcs#324
Also, preserve the duplication if we update or modify the package.json file. Fix: npm/rfcs#324
* [#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))
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
:bar/package.json
:How
Current Behaviour
Currently
npm
ignorespeerDependencies
definition and treatsfoo
as a regular dependency. This results innode_modules/left-pad@1.1.0
andbar/node_modules/left-pad@1.3.0
to be installed.Desired Behaviour
It would be great if
npm
gave priority topeerDependencies
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 byexample
and bybar
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
andpeerDependencies
without a need from a user to make a decision how to satisfy thesepeerDependencies
. 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 rootpackage.json
.References
The text was updated successfully, but these errors were encountered: