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] peerDependencies not preferred over dependencies when both are present #7106

Open
2 tasks done
quantizor opened this issue Dec 27, 2023 · 7 comments
Open
2 tasks done
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x

Comments

@quantizor
Copy link

quantizor commented Dec 27, 2023

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

Here is a configuration for the styled-components package.json that I'm testing:
Screenshot 2023-12-27 at 2 45 46 PM

The design goal of this is to provide a dependency version specified by us, but allow it to be overridden by peerDependency in client projects if their installed version is greater than ours. Looking at this RFC that was closed as completed, it seems like this is meant to work in npm. However, both versions are currently installed. Here is an example repo demonstrating the current behavior: https://github.com/quantizor/styled-components-repro

I did an analysis of other package managers like yarn and pnpm, and they both seem to honor peerDependencies as an override over dependencies if both are specified in a library being consumed by a client application.

You can see this in action for the supplied reproduction repository by following these instructions after cloning:

  1. Delete node_modules and package-lock.json
  2. Run yarn install
  3. Observe in node_modules/styled-components that no extra node_modules folder is added

Expected Behavior

If a package declared in both dependencies and peerDependencies, peerDependencies should "win" if a compliant higher version is installed in the client application.

Steps To Reproduce

  1. Clone https://github.com/quantizor/styled-components-repro and cd into the directory
  2. Run npm i
  3. Observe that node_modules/styled-components/node_modules has a duplicated module for postcss (there is a higher version also installed at node_modules/postcss)

Environment

  • npm: 10.2.5
  • Node.js: 18.19.0
  • OS Name: macOS
  • System Model Name: MacBook Air
  • npm config:
; node version = v18.19.0
; npm local prefix = /Users/[redacted]/code/styled-components-repro
; npm version = 10.2.5
; cwd = /Users/[redacted]/code/styled-components-repro
; HOME = /Users/[redacted]
@quantizor quantizor added Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x labels Dec 27, 2023
@ljharb
Copy link
Contributor

ljharb commented Dec 27, 2023

What I would expect in this case, and what I believe npm has always done, is to install the "dependencies" range, but if it does not agree with the peerDependencies range, npm ls will fail, and npm 7+ will warn and/or fail during installation.

Typically whenever I have a peer dep listed, I put the identical range either in deps, or devDeps.

With your example, what does npm 6 do? Does it install =3.2.0, or does it install the latest v3?

@quantizor
Copy link
Author

With your example, what does npm 6 do

It does the expected behavior, it looks like npm >= 7 regressed/changed things. The only extra module installed in node_modules/styled-components/node_modules for npm v6 is tslib which might be correct since tslib isn't an explicit dependency in the client package.json.

I put the identical range either in deps, or devDeps.

This isn't safe though. We want to have a pinned version specified as a safe, integration-tested default with the client able to install a greater version if they choose as long as it falls within the allowed semver range specified in peerDependencies.

@ljharb
Copy link
Contributor

ljharb commented Dec 27, 2023

It's quite safe; that's what a lockfile is for. Nothing should be pinned in package.json, ideally.

@quantizor
Copy link
Author

What's allowed in peerDependencies is more expressive than what can go in dependencies. For example, the react peer dep entry is ^16.8.0 || ^17.0.0 || ^18.0.0 which would be nonsense for dependencies.

All I'm trying to get across is that there are scenarios where they can't match and if you review the NPM RFC I linked in the OP (I know you were part of the conversation way back when but it's been a while), Isaac agreed that peer dependency entries should take precedence if both are specified.

@ljharb
Copy link
Contributor

ljharb commented Dec 27, 2023

That wouldn't be nonsense in deps at all; I do exactly that quite often and it works fine.

@quantizor
Copy link
Author

Ok, what about the second paragraph though.

@ljharb
Copy link
Contributor

ljharb commented Dec 27, 2023

While I still feel that the dep/devDep range should be used for installation instead of the peerDep range, as I indicated in #7106 (comment), I do agree with you that the closed and accepted RFC you linked suggests that things should work as you indicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x
Projects
None yet
Development

No branches or pull requests

2 participants