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

peerDependencies aren't bumped #333

Closed
5 tasks done
StarpTech opened this issue Sep 9, 2022 · 5 comments · Fixed by #363
Closed
5 tasks done

peerDependencies aren't bumped #333

StarpTech opened this issue Sep 9, 2022 · 5 comments · Fixed by #363

Comments

@StarpTech
Copy link
Contributor

StarpTech commented Sep 9, 2022

Describe the bug

Hi, I have two packages A,B. B has a peerDependency to A.

{
  "name": "B",
  "peerDependencies": {
    "A": "workspace:^0.105.1"
  },
  "dependencies": {
    "A": "workspace:^0.105.1"
   }
}

After publishing A to 0.106.0 the version in dependencies was updated correctly but not for the peer-dependency.
I tried it as fixed constraints 0.105.0 and without the workspace protocol. In all cases, the peer-dependency was not bumped.

See lerna/lerna#955

Expectation

I'd expect that the peer dependency is updated to the next compatible version based on the version constraint.

A bumps with semver ^0.105.0 to 0.105.2 => B bumps with semver ^0.105.0 => 0.105.2 ✔️
A bumps with semver 0.105.0 to 0.106.0 => B bumps with semver 0.105.0 => 0.106.0 ❌ because it is not compatible

Reproduction

See above.

Lerna config and logs

lerna.json

<!-- Please paste your `lerna.json` here -->

lerna-debug.log

<!-- If you have a `lerna-debug.log` available, please paste it here -->
<!-- Otherwise, feel free to delete this <details> block -->

Environment Info

| Executable        | Version |
| ----------------- | ------- |
| `lerna --version` | VERSION |
| `npm --version`   | VERSION |
| `yarn --version`  | VERSION |
| `node --version`  | VERSION |

---
OR simply run `npx lerna info` command and copy+paste the result here

Used Package Manager

pnpm

Validations

@ghiscoding
Copy link
Member

ghiscoding commented Sep 13, 2022

If we're reading a few of the comments associated to the issue you referenced, it seems that the original Lerna's maintainer (evocateur) purposely left it this way because it's a rather sensitive subject that can be interpreted differently by many users. Basically this post is explaining it broadly

peer deps should never be forcefully updated; updating a minimum peer dep range is a breaking change. Peer dep ranges should always remain as broad as is possible.

and then if we scroll up a little and we click on the issue right above that post which is this issue

peerDependencies shouldn't be updated by lerna publish

and we start reading what everyone are saying, we are confronted with a lot of different opinions on the subject but a lot of them seems to think that we shouldn't bump peer dependencies and that goes against your issue

In summary, I'm not so sure that I should deviate from the same implementation that Lerna already does, at this point in time, the code in Lerna-Lite follows the same logic as Lerna (it's worth to mention again that evocateur himself didn't want to change/fix the issue you referenced). I think the best I could do would be to implement 1 of these possible suggestions

  1. update only if it doesn't detect a version range that includes an operator (ie: "a": ">=1.0.0")
  2. provide a flag to force update peer dependencies (not sure how to call it though, please provide name suggestion)
    • to be clear the force update would only work when the bump is actually possible
  3. we could also go for a mix of 1 and 2, that is when the flag is enabled and we have an version range
    • something along --allow-bumping-peer-deps-without-range

I would rather go with option 2 or 3 which is to add a flag (need a name) and that wouldn't be too hard to implement in Lerna-Lite because I had already extracted the deps update into a new separate function retrievePackageDependencies to fix another peer deps issue you had opened a few months back, so I basically just need to provide true as the 2nd argument to this new function and that might just be enough but I don't want to implement it for everyone by default so we would have to go with 1 of these 3 approaches

https://github.com/ghiscoding/lerna-lite/blob/9815d64388e14e3eb3eb1970c3a083c0dcb54ac2/packages/core/src/package.ts#L361-L384

@StarpTech
Copy link
Contributor Author

Thank you for the explanation. I'll come up with a proposal.

@ghiscoding
Copy link
Member

ghiscoding commented Sep 13, 2022

I tested a few code change and found out that there's actually another indirect issue in both Lerna/Lerna-Lite. The way that the code is implemented is that it will only update the first dependency that it finds (following the order: 1. dependencies, 2. localDependencies, 3. devDependencies and 4. peerDependencies) and so if we define the same dep in both dependencies and peerDependencies, well it will only ever update the first one and never the peer one. So I think adding an extra loop just for the peer deps might be required to fix this problem

in other words, if we take the demo you have

{
  "name": "B",
  "peerDependencies": {
    "A": "workspace:^0.105.1"
  },
  "dependencies": {
    "A": "workspace:^0.105.1"
   }
}

it would only update the first dependencies because of its primary order even though defining both of these properties should be totally fine and acceptable (technically speaking, it's only fine for duplicate it when adding a peer deps, while any other time we should not allow to repeat the same pkg)

@ghiscoding
Copy link
Member

ghiscoding commented Oct 4, 2022

@StarpTech could you please review the PR #363 which will close this issue

@StarpTech
Copy link
Contributor Author

Yes, thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants