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

feat(version): add --allow-peer-dependencies-update, closes #333 #363

Merged
merged 14 commits into from Oct 14, 2022

Conversation

ghiscoding
Copy link
Member

@ghiscoding ghiscoding commented Oct 5, 2022

Description

Add a new option --allow-peer-dependencies-update option on the version command which will help to update peer dependencies before publishing them. This option is opt-in and disabled by default because of what the original Lerna maintainer wrote in Lerna issue #1018

peerDependencies shouldn't be updated by lerna publish

This option is not recommended for most users, for them peerDependencies will remain untouched unless the user really wants it enabled and has more knowledge in this regard. However even when this flag is enabled, it will never mutate/update peer dependencies with semver range operator (ie >=2.0.0).

Regex used

For this implementation, we'll use this regex to identify if the peer dependency is allowed to be updated or not (regex101)

/^(workspace:)?[~^]?[\d\.]+([\-]+[\w\.\-\+]+)*$/i

Note

While at it, the code in package and package-graph related to workspace: protocol were refactored a bit to follow similar implementation as to what Lerna as recently done, even though the implementation is different at least the naming is now similar.

Motivation and Context

Provide a way to bump peer dependencies but only via a flag, this behavior will be disabled by default, this will close #333

The main code change for this new option is in the updateLocalDependency() method found in package.ts, a new argument allowPeerDepsUpdate was added and when enabled it will add peerDependencies into the loop of dependencies to be inspected for updates. The loop is also new in that same method, since prior to this we would only update/bump the 1st dependency found, however now if the flag is enabled, we also include (loop) the peer dependency as well so instead of updating 1 dependency (by name), we might update up to 2 of with the same name

in summary, calling lerna version --allow-peer-dependencies-update (or the equivalent lerna.json config) on this package

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

will bump both the dependencies and peerDependencies to ^1.3.0 on a minor bump (note again that a semver range >= will never be bumped)

How Has This Been Tested?

added a lot more unit tests to cover package.ts, package-graph, publish and version

What will be valid and update?

It will update (bump) regular semver like these

  • 1.2.3
  • ^1.2.3
  • ^1.4.0-alpha.0
  • workspace:^1.2.3

but it won't mutate/update things like

  • >=1.0.0
  • >=1.0.0 <2.0.0
  • ^1 | ^2 | ^3

Types of changes

  • Chore (change that has absolutely no effect on users)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ghiscoding ghiscoding changed the title Feat/update peer dependencies feat(version): add --allow-peer-deps-update, closes #333 Oct 5, 2022
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #363 (1d1b9ba) into main (a5d2928) will increase coverage by 0.91%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
+ Coverage   96.35%   97.26%   +0.91%     
==========================================
  Files         145      145              
  Lines        4271     4296      +25     
  Branches      978      992      +14     
==========================================
+ Hits         4115     4178      +63     
+ Misses        156      118      -38     
Impacted Files Coverage Δ
packages/core/src/command.ts 100.00% <100.00%> (+5.89%) ⬆️
packages/core/src/package-graph/package-graph.ts 100.00% <100.00%> (ø)
packages/core/src/package.ts 100.00% <100.00%> (+0.63%) ⬆️
packages/core/src/utils/query-graph.ts 100.00% <100.00%> (ø)
packages/publish/src/publish-command.ts 99.03% <100.00%> (+0.01%) ⬆️
packages/version/src/version-command.ts 100.00% <100.00%> (ø)
packages/run/src/run-command.ts 100.00% <0.00%> (ø)
packages/publish/src/lib/npm-publish.ts 100.00% <0.00%> (ø)
packages/optional-cmd-common/src/lib/profiler.ts 100.00% <0.00%> (ø)
packages/cli/src/cli-commands/cli-run-commands.ts 100.00% <0.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ghiscoding ghiscoding mentioned this pull request Oct 5, 2022
5 tasks
@ghiscoding ghiscoding force-pushed the feat/update-peer-dependencies branch from 9369a33 to d8b846f Compare October 6, 2022 00:10
@ghiscoding
Copy link
Member Author

@StarpTech are you going to have a chance to review the PR within the next couple days? I'd like to release this by next week if possible

@StarpTech
Copy link
Contributor

I try my best to review it this weekend 🤞

@ghiscoding
Copy link
Member Author

I wonder if I should rename the option name --allow-peer-deps-update which is shorter to a longer but more precise option name --allow-peer-dependencies-update to make it clearer, it seems quite long though

@ghiscoding ghiscoding changed the title feat(version): add --allow-peer-deps-update, closes #333 feat(version): add --allow-peer-dependencies-update, closes #333 Oct 12, 2022
@StarpTech
Copy link
Contributor

I'm going to review it today.

@ghiscoding
Copy link
Member Author

@StarpTech that'd be awesome because I was about to merge without review, I want to release by the end of this week. Thanks

@StarpTech
Copy link
Contributor

--allow-peer-deps-update

It is pretty clear to me. I like.

@StarpTech
Copy link
Contributor

StarpTech commented Oct 12, 2022

For this implementation, we'll use this regex to identify if the peer dependency is allowed to be updated or not (regex101)

I'm not sure if this is right. A fixed peerDep 1.0.0 should also not be updated.


Hi, after reading other issues, I'm unsure if the current assumptions are correct in independent mode. My issue in #333 was the too-restricted version constraint. Under < v1 the developer can use > 0.1.0 < 1 to simplify the releases because no stability is guaranteed. Starting with v1 he can use ^1.0.0 to not break the public API.

With that knowledge, I'd expect that Lerna is throwing an error (before release and commit) when a version is being published with an invalid peerDep constraint. The developer is responsible to fix this manually.

In global mode, this may be automated by bumping the peerDep to lowest compatible version. Again the developer is responsible to set a proper range. Otherwise, Lerna would update it every time.

Does it make sense?

@ghiscoding
Copy link
Member Author

ghiscoding commented Oct 12, 2022

For this implementation, we'll use this regex to identify if the peer dependency is allowed to be updated or not (regex101)

I'm not sure if this is right. A fixed peerDep 1.0.0 should also not be updated.

in this case the peer dep 1.0.0 update is allowed and would follow the exact same logic as a regular dependencies would, the bump logic did not change at all in this PR. The only change this PR does is to duplicate the bump from the regular dependencies into the peer dependencies as well when the option is enabled, that's it.

Hi, after reading other issues, I'm unsure if the current assumptions are correct in independent mode. My issue in #333 was the too-restricted version constraint. Under < v1 the developer can use > 0.1.0 < 1 to simplify the releases because no stability is guaranteed. Starting with v1 he can use ^1.0.0 to not break the public API.

I'm not entirely sure of everything you said but technically speaking the new option would not bump something like > 0.1.0 < 1 because the option would not pass through the regex check because it contains an operator, you can see some examples in the description above of what would go through and what won't

In global mode, this may be automated by bumping the peerDep to lowest compatible version. Again the developer is responsible to set a proper range. Otherwise, Lerna would update it every time.

I don't think it's Lerna's responsibility to do anything with a peer dependency that has version range with operator(s), that is the user responsibility to make it valid. The goal of this new PR is simply to apply the exact same bump, as a regular dependency would, and duplicate it in the peer dependencies if need be (which is exactly what you described in issue #333), and that will fix the main issue that everyone brought up in Lerna. Also the regex that I mentioned is only in place to act as a guard (as a peer dep, are you allowed to be bumped, yes or not), this PR has zero code change for the bump logic, I don't intent to change that.

To explain a little more what this PR does, prior to this PR the previous implementation of updateLocalDependency() would apply the update (bump) to the first entry it found. However if the user has a package in the regular dependencies (or any other deps like dev, local, ...), then it would never reach the peerDependencies because it used to "first found, first assigned". So what this PR does is to convert the unique update into an array of up to 2 update (a regular dependency and possibly a peer dependency) and that's about it as to what this PR does.

@StarpTech
Copy link
Contributor

You probably have a better understanding of this than me. I'm no longer convinced when this feature is required. Could you add some simple use cases? It might also be good to mention them in the README. After reading the README, the feature sounds like a super edge case.

@ghiscoding
Copy link
Member Author

ghiscoding commented Oct 13, 2022

@StarpTech well the perfect example would be the one you provided in #333, I also updated the version readme, as suggested, with info shown below

with the new flag both deps would be bumped, for example a minor

{
  "name": "B",
  "dependencies": {
    "A": "workspace:^1.2.0"   // will bump to "workspace:^1.3.0",
    "B": "^0.4.0":            // will bump to "^0.5.0"
   },
  "peerDependencies": {
    "A": "workspace:^1.2.0"   // will bump to "workspace:^1.3.0"
    "B": ">=0.2.0":           // will not be bumped because range with operator (>=) are skipped
  }
}

without the flag it will only bump the first one it found, that is dependencies in this case, so peer deps would never be bumped (which is exactly the issue you opened #333)

{
  "name": "B",
  "dependencies": {
    "A": "workspace:^1.2.0"   // will bump to "workspace:^1.3.0"
    "B": "^0.4.0":            // will bump to "^0.5.0"
   },
  "peerDependencies": {
    "A": "workspace:^1.2.0"   // will NEVER be bumped
    "B": ">=0.2.0":           // will NEVER be bumped
  }
}

I think the concept from the first implementation (prior to this PR) was that you can never have the same package dep into more than one dependencies (or dev, local, ...) and for that reason it was only ever updating the first one it found. With that in mind, if you had pkg-1 in two areas (dependencies and devDependencies) then Lerna was considering this has an error by the user's side and was only ever updating the first deps section it found with the package name (dependencies in this case, also note that there's an order in which one it updates first, starts with 1. dependencies, 2. optionalDependencies, 3. devDependencies, 4. peerDependencies).

However, peerDependencies is the exception here because that is totally valid to have both a dependencies and a peerDependencies that includes the same package name, but this was never considered in Lerna and they also said that peer deps should never be touched anyway, so why bother!? This PR fixes this exception but only for peer deps, meaning that if you have the same pkg-1 into dependencies, devDependencies and peerDependencies, it will update the first one (dependencies) and the peerDependencies but it will never update the dev one because it shouldn't be there anyway

@ghiscoding ghiscoding merged commit efaf011 into main Oct 14, 2022
@ghiscoding ghiscoding deleted the feat/update-peer-dependencies branch October 14, 2022 19:14
@ghiscoding
Copy link
Member Author

I think my implementation is ok, so I'll go ahead and merge the PR, I'll release a new version probably later today

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

Successfully merging this pull request may close these issues.

peerDependencies aren't bumped
2 participants