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 shouldn't be updated by lerna publish #1018

Closed
ianstormtaylor opened this issue Sep 18, 2017 · 53 comments
Closed

peerDependencies shouldn't be updated by lerna publish #1018

ianstormtaylor opened this issue Sep 18, 2017 · 53 comments

Comments

@ianstormtaylor
Copy link

ianstormtaylor commented Sep 18, 2017

Given the following setup and using independent versioning:

packages
| -- foo
| -- bar
// foo/package.json
{
  "version" "0.2.1"
}
// bar/package.json
{
  "peerDependencies": {
    "foo": ">=0.2.0"
  },
  "devDependencies": {
    "foo": "^0.2.1"
  }
}

If you make changes in foo, and run lerna publish (patch)...

  1. foo will updated to 0.2.2, as expected.
  2. bar's dev dependency on foo will be updated to ^0.2.2, also as expected.
  3. But bar's peer depdenncy on foo will be overwritten to ^0.2.2, unexpectedly!

I think the peerDependencies tracking of Lerna should be disabled, since it's impossible for Lerna to know whether a change would have forced a peer dependency upgrade or not.

Peer dependencies should remain as loose as possible, with the absolute earliest accepted version that works, reducing warning noise and increasing interop. With the current logic of forcing a bump with each publish, Lerna forces dependents to upgrade even if there's no need to.

Related to #955

@ljharb
Copy link

ljharb commented Sep 26, 2017

peer deps should definitely be loose; any peer dep change that removes a valid version is semver-major.

@ianstormtaylor
Copy link
Author

Also, I forgot there's another unexpected thing...

Because of the unexpected bump of the peer dependencies, when you make a small change to the "core" package that is peer-depended on, Lerna will think that any packages that have a peer dependency on it have also been "changed", so when you go to publish you'll be publishing changes for every package, instead of just the one you actually changed.

Can any maintainers comment on this?

As far as I can tell, using independent versioning with peerDependencies is impossible without a broken UX for semver...

@evocateur
Copy link
Member

I agree that peer dependencies, when used at all, should be as flexible as possible. The current handling of peer dependencies in lerna could use some improvement, definitely.

That said, if you aren't also expressing a dev dependency on a sibling package that you've expressed a peer dependency for, how are you even running your unit tests? Peer dependencies aren't installed by default, after all. (This pattern applies for all peer dependencies, not just lerna siblings)

The "increment version of sibling consumer when a sibling dependency changes" behavior has been discussed in detail several times, but the TL;DR: versions are cheap, it's not a violation of semver to express when dependencies have updated, and it's really not worth worrying about, tbh.

@ljharb
Copy link

ljharb commented Sep 29, 2017

@evocateur indeed, the best practice is a) for peer dep ranges to be as wide as possible, and b) for dev dep ranges to be identical to the peer dep range.

Updating deps isn't a violation of semver; updating the bottom of the range for peer deps - because it's part of the API of the module - is absolutely semver-major.

airbnb's enzyme recently switched to lerna; and this is a reason we may want to abandon it - it's that important, and decidedly "worth worrying about".

@evocateur
Copy link
Member

@ljharb Ah yes, that clarification I agree with, thank you.

Updating deps isn't a violation of semver; updating the bottom of the range for peer deps - because it's part of the API of the module - is absolutely semver-major.

The "worth worrying about" bit was not specifically aimed at peerDependencies, but the penchant for lerna to publish new versions of a dependent when the dependency changes.

In fact, I'm fully on-board the "ignore peerDependencies in lerna publish" boat, at this point. The dev dependency will need to be bumped as before, because that's currently how lerna recognizes a matching sibling that needs a local relative symlink, etc. Does simply abandoning any modification of peerDependencies work for enzyme?

@ljharb
Copy link

ljharb commented Sep 29, 2017

That's certainly better then the current situation! However, what would be ideal is expanding the existing peer dep range so that it includes the new dev dep version; and separately, it would be ideal to find a way to keep the dev dep matching the peer dep, but that doesn't have to be a blocker.

@ianstormtaylor
Copy link
Author

Thanks for the input @evocateur! I'd definitely love the "abandon" approach for a quick solution.

And then later we could get smarter with trying to keep them in sync with the new maximum dev dep. This seems like it's going to be somewhat rare anyways, and easy to fix in a quick patch when you forget, so it's not a big issue like the current setup.

@ljharb
Copy link

ljharb commented Sep 30, 2017

@ianstormtaylor fwiw it's not rare, it's "every package in the react and eslint ecosystems", plus any other ecosystem that uses peer deps :-)

@ianstormtaylor
Copy link
Author

Yeah I guess that's true, I should have expanded.

I'm actually of the opinion that peerDependencies shouldn't be touched at all though, because unless I'm missing something, Lerna will only ever be able to guess at what they should be changed to. If you have many independent packages and you major bump one of the core ones, there's a very good chance that you need to do two different things to peer dependencies:

  1. Completely bump them for packages that depend on the new changes.
  2. Leave them loose, but include the new major for ones that don't.

It feels like Lerna trying to do anything magical here is just going to complicate the issue. I'd rather just have full control over what is pegged.

Although, regardless it sounds like we're agreed that doing nothing would be a great first step.

@ljharb
Copy link

ljharb commented Oct 2, 2017

Maybe I should also expand :-)

Let's say the peer dep range is ^4 || ^5, and you're otherwise bumping to v6. The peer dep range must become ^4 || ^5 || ^6 if it's going to retain back compat while also allowing v6. If the range is ~15.3 || ~15.4, and you're otherwise bumping to 15.5, then the range must become ~15.3 || ~15.4 || ~15.5.

What might be a viable alternative beyond doing nothing (indeed a good first step) is an in-terminal UI to allow me to provide a peer dep range, if lerna is unable to confidently come up with one?

@ianstormtaylor
Copy link
Author

ianstormtaylor commented Oct 2, 2017

That makes sense, but my point is that Lerna can't actually know whether a package wants to extend itself to include the new peer dep and retain backwards compat, or whether it wants to start fresh with only the latest version included.

I think trying to do anything magic here is going to end up being wrong more often than it's worth, especially when you consider it across multiple packages.

An in-terminal UI prompt would be great though! It could even just ask you if you want to "extend" or "reset" the peer dep range, potentially without having to ask you to remember the exact ranges to set.

That said, I think we should hold off on that and first just get Lerna to stop touching peer deps.

@Dru89
Copy link

Dru89 commented Nov 21, 2017

That said, I think we should hold off on that and first just get Lerna to stop touching peer deps.

Sorry for being dense, but I see that we have the "help wanted" label applied here. Would this literally just be removing this line?

this.updatePackageDepsObject(pkg, "peerDependencies", exact);

Or would we prefer to move forward with a UI prompt to update peer dependencies? I'm in full agreement that we shouldn't use any magic or special circumstances to try to decide for the user what to do on the peerDependencies fields.

@ianstormtaylor
Copy link
Author

@Dru89 I'm not familiar with the Lerna codebase, so I'm not sure what's involved on that front, but I think we're in agreement that the first step is to remove peerDependencies logic. And if someone wants to separately take on some sort of UI that can be done afterwards. Thank you!

evocateur added a commit to evocateur/lerna that referenced this issue Jan 9, 2018
Changes to a peer version range are always semver major, and should be as broad as possible.

refs lerna#1018
@evocateur
Copy link
Member

#1187 is the first step toward resolving this, stopping lerna publish from making inappropriate semver-major changes to any local peerDependencies.

evocateur added a commit that referenced this issue Jan 9, 2018
Changes to a peer version range are always semver major, and should be as broad as possible.

refs #1018
jshcrowthe added a commit to firebase/firebase-js-sdk that referenced this issue Jan 22, 2018
jshcrowthe added a commit to firebase/firebase-js-sdk that referenced this issue Jan 25, 2018
* Update inquirer version

* Initial work on automating release process

* Fix array index mismatch error

* Add ability to auto commit and tag

* Refactoring util functions

* Add step to clean git tree

* Fix missing fxn

* Change to force mode

* Refactor git clean to use exec

* Reorder steps to not commit changes till later, add rebuild and test processes

* Adding indicators to long running processes

* Add missing dep

* Build in reset mechanism

* Adding in a log statement

* Refactoring the test setup stuff

* Testing the testing

* Fixing scoping issue

* Fixing another scoping issue

* Fixing the result is undefined issue

* Refactor from exec -> spawn

* Expose test output to the console

* Revert "Expose test output to the console"

This reverts commit d0ae119.

* Refactor so we don't use a spinner for the test run

* Adding the version check

* Updating the CLI to tag and push releases

* Initial work on NPM publish

* Continuing work on npm publish

* Redirect npm publish stdio

* Add some logging to the NPM publish

* More logging

* Add newline to end of JSON files

* Remove disabling comments

* Remove "dry-run" stuff

* [AUTOMATED]: Prettier Code Styling

* [AUTOMATED]: License Headers

* Remove the old publish scripts

* [AUTOMATED]: Prettier Code Styling

* Fix issue where validation wasn't properly being captured

* Add missing push verification

* [AUTOMATED]: Prettier Code Styling

* Add banner text

* [AUTOMATED]: Prettier Code Styling

* Changing publish to fail fast

* Add npm whoami check

* Add .yarnrc

* Remove logic to update peerDependencies

See: lerna/lerna#1018 for rationale.

* for...in -> for...of

* Point .yarnrc to public NPM registry

* [AUTOMATED]: Prettier Code Styling

* Add ability to get releaseType from command line args

* Fix await/async issue

* Refactor to handle canary builds

* Update version names and associated tagging

* Remove early return (for debugging)

* Update .yarnrc for testing

* Add try-catch to async fxn

* Change error messaging

* [AUTOMATED]: Prettier Code Styling

* Refactor to exclude prerelease versions from the git log

* More canary updates

* Refactor to only push to github on prod releases

* Add error logging

* Fix misassigned var issue

* Pass raw releaseType

* Fix private package issue

* Removing trim() call

* Remove log

* Add comment describing canary versioning

* Update comments

* Removing early return

* Temporarily disabling validation

* Fix scoping issue

* Add auto-reset to the working tree for non-production releases

* Fetch an uncached version of the pkg.json for final push

* Fix whitespace issues, filter to public packages

* Parallelize the publishing of packages

* Fix issues

* Refactor to show parallel spinners

* Return Listr obj

* Fix a bad variable ref

* Run the publish

* Pass the proper variable along

* Add a console log

* Adding some whitespace

* Refactor logs

* [AUTOMATED]: Prettier Code Styling

* Add git validation before release

* Compare git sha's before allowing publish

* Compare git sha's before allowing publish

* Debugging

* Fix conditional

* Remove unneeded log

* Refactor to get the latest master sha

* Remove "at head" check

* [AUTOMATED]: Prettier Code Styling

* Update .yarnrc to point to remote NPM

* Re-enable build verification

* Travis CI canary builds

* Updating yarn.lock (autogenerated)

* [AUTOMATED]: Prettier Code Styling

* Copy of master yarn.lock

* Commiting isolated yarn.lock changes

* [AUTOMATED]: Prettier Code Styling

* Refactor to do a single script value as travis doesn't support multiple scripts

* Refactor root -> projectRoot from @hiranya911

* Add logging around npm whoami

* Removing comma

* [AUTOMATED]: Prettier Code Styling

* Refactor production push verification order

* Adding some notes to the process

* [AUTOMATED]: Prettier Code Styling
@sliekens
Copy link

@evocateur so what is the intended workflow at this point for updating the version range of local peerDependencies when they receive a major version bump?

My project depends on the old behavior where Lerna automatically increases the lower bound of the version range of local peerDependencies whenever their version is incremented. It would be nice if this behavior could be made available again with an optional command-line switch.

@sliekens
Copy link

sliekens commented Jan 30, 2018

Given the original example:

// bar/package.json
{
  "peerDependencies": {
    "foo": ">=0.2.0"
  },
  "devDependencies": {
    "foo": "^0.2.1"
  }
}

If you'd commit a breaking change to foo and publish with lerna 2.7+ the bar package gets published with an updated devDependency version but with the same old peerDependency version.

// bar/package.json
{
  "peerDependencies": {
    "foo": ">=0.2.0"
  },
  "devDependencies": {
    "foo": "^1.0.0"
  }
}

Now assume that the update from foo@0.2.1 to foo@1.0.0 is so disruptive that you had to rewrite the entire bar package. Under that assumption, the version range for foo@>=0.2.0 is wrong because any foo older than 1.0.0 most likely won't work...

So given the original requirement:

Peer dependencies should remain as loose as possible, with the absolute earliest accepted version that works

Simply removing tracking of peerDependencies does not satisfy the last part of the requirement.

@ianstormtaylor
Copy link
Author

@StevenLiekens you need to do that change by hand, it's not something that Lerna can infer for you since it could just have easily been allowed to stay the same.

If you're like me though, the previous bumping logic was causing the bump every time you made any change to the dev dependency at all, which was causing what was a essentially an undocumented breaking change to your bar package each time foo was published.

@ljharb
Copy link

ljharb commented Jan 30, 2018

It's something that could probably be configured so that lerna doesn't have to infer it, though.

@ianstormtaylor
Copy link
Author

@ljharb true, totally agree! As long as the default is to not do any magic that might contain breaking changes in it I'm good with whatever.

Hypnosphi added a commit to storybookjs/storybook that referenced this issue Mar 10, 2018
chancancode pushed a commit to CondeNast/cross-check that referenced this issue Jul 3, 2018
This is necessary because lerna/lerna#1018

We don't agree with the sentiment on the thread.
@sliekens
Copy link

sliekens commented Aug 1, 2018

Bump because I still think you need to keep peer dependencies in sync with dev dependencies somehow. Either by automatically modifying the peer dependency range if there's a conflict with the dev dependency, or by refusing to publish with a conflicting dev dependency.

We've been having issues ever since this patch where we have a dev+peerdependency on foo@^1.0.0 and lerna automatically changes the devdependency to foo@2.0.0 . This sucks because we don't even notice right away when it happens. (because we're doing conventional commits + continuous delivery)

If the goal was to reduce warning noise and increase interop, this is not it.

@ianstormtaylor
Copy link
Author

Having a warning if the dev dependency isn't satisfied by the peer dependency sounds like the right way to go to me. However, don't you already have these warnings when you run yarn install or npm install? I'd think they already are there.

@sliekens
Copy link

sliekens commented Aug 1, 2018

We don't get the warnings while developing packages (because yarn/npm install ignores peerdependencies).

We do get warnings when installing the released packages into other projects.

After updating
foo 1.0.0 -> 2.0.0
bar 1.0.0 -> 1.0.1

warning "bar@1.0.1" has unmet peer dependency "foo@^1.0.0"

But in reality bar is compatible with foo@2.0.0 and sometimes even requires it when we decide to break backwards compatibility.

foo 1.0.0 -> 2.0.0
bar 1.0.0 -> 2.0.0

warning "bar@2.0.0" has unmet peer dependency "foo@^1.0.0"

I hope that makes sense.

@evocateur
Copy link
Member

Seems like you'd need to prompt the user for a decision at some point. peerDependencies are fraught with hidden assumptions/expectations.

@sliekens
Copy link

sliekens commented Aug 1, 2018

Yes!! Except our release pipeline is completely automatic and noninteractive so actually I'd prefer if the publish command would scan for conflicts and crash if it finds any.

@ianstormtaylor
Copy link
Author

@StevenLiekens why are you not declaring the peer dependency as >=1.0.0 instead of ^1.0.0? Sounds like that would solve all your problems. That's actually how all peer dependencies should be written, until a newer version comes along that is actually backwards incompatible.

@sliekens
Copy link

sliekens commented Aug 1, 2018

That would allow consumers to update the depended-on-package without also updating the dependent package and it wouldn't ever trigger warnings.

@ianstormtaylor
Copy link
Author

@StevenLiekens yeah, which is often a feature and not a bug. People don't often peg node versions with ^ in engines (or at least they shouldn't) because if everyone did that then we'd need to publish a new version of every npm package every time a new version of node landed.

The same goes for the React ecosystem. Tons of people are wasting effort and time because they pegged their react peer dependency to ^0.14.0 when their components were not impacted at all by the move to 15.0.0, and same again with 16.0.0. And then each time a new React version comes out everyone freaks out and thousands of pull requests have to be merged and packages re-published.

The reality is that when breaking changes necessitate major version bumps, often it's just a single part of the API that has changed, and for any given module the chance is low that it breaks with it. This is probably even more true for internal packages managed with Lerna (or at least from what I've found).

And when you have a package that does actually have that break, you can just restrict the peer dependency range in a patch fix and publish the new restriction so warnings are given.


I'm not arguing against adding a warning to help if you really don't want to use >= for some reason. Although I don't personally need it and it sounds like others don't either, so you'll probably need to write your own pull request if you want to see it landed. Ideally it would be handled automatically with the existing yarn/npm warnings I'd think though.

@sliekens
Copy link

sliekens commented Aug 1, 2018

Okay so you prefer convenience over correctness.

I have one big problem with >= and it's that npm (and yarn?) defaults to installing the highest version in a range on new installs.

If you publish a package today that depends on react >= 16.0.0 then someone who installs it 5 years from now will get whatever react will be the latest version in 2023 and there is just no way you can guarantee today that that will still work. But your package says it should work and that's a real problem because now even tools won't warn you that it may break.

Now imagine all the wasted time and effort if tools stop warning about version mismatches and everyone has to figure out for themselves if the packages they depend on are actually compatible or not.

I think correctness is still the right answer although I agree it means doing a lot of unnecessary work.

/edit to say that this doesn't really apply to peerdependencies except the part about there not being a warning when there PROBABLY should be

@ianstormtaylor
Copy link
Author

ianstormtaylor commented Aug 1, 2018

@StevenLiekens it's pragmatic, yup.

Arguing for always going with the purest, "correct" pegging of peer dependencies is naive I think because it doesn't take into account maintenance burden. It all depends on the stability of the APIs you're building for, and the rate at which new versions as released. If the APIs are stable and development of new versions is continuous, you're going to want to use >=. If not, and APIs are unstable, ^ is better.

You can see this in the node community, where almost all of the top packages use >= pegging for the engines field. (browserify, express, grunt, gulp, lodash, request, chalk, forever, etc.)

Whereas in the React ecosystem things are less standardized. More popular packages seem to use ^ (react-select, react-motion, react-intl, react-apollo, etc.), but there are still popular packages that use >= as well (react-router, react-helmet, etc.)

While new node versions require almost no extra work for people to republish packages, new React versions cause huge frenzies while every single one of the packages using ^ has to be re-published (across all of its active major versions) for the new React. I think it's a huge waste of time considering how stable React's APIs are, but it's their call.


Either way, you have to decide for yourself how stable your own monorepo's APIs are.

This issue is solved for me since peerDependencies are no longer being auto-incremented and causing breakages. Feel free to open another for your concerns to be addressed.

@ljharb
Copy link

ljharb commented Aug 2, 2018

For the record, using >= is insanely dangerous and reckless and is absolutely an antipattern. It’s not pragmatism or convenience to set yourself up for guaranteed future breakage when the next semver-major is released.

@sliekens
Copy link

sliekens commented Aug 2, 2018

With >= you just reduce the maintenance burden, not eliminate it. You still have to implement fixes and update the version range when things do break.

And when things break, they break hard. You can release a patch version that fixes compatibility but your older packages will be forever broken.

Oh and tools won't warn about it so you have to tell your users not to use anything older than x.y.z. Also have fun explaining to your users that they're using the wrong version when bug reports start coming in.

@sliekens
Copy link

sliekens commented Aug 7, 2018

Here's another thing that's been really bothering me and it applies to every kind of version range.

Consider this:

// bar/package.json
{
  "peerDependencies": {
    "foo": ">=0.2.0"
  },
  "devDependencies": {
    "foo": "0.7.8"
  }
}

Assume that foo@0.7.8 contains 5 features and 8+ fixes that are not available in foo@0.2.0.

How do you guarantee that your own code does not depend on any of these features and fixes?

Personally I think you can't unless you take a hard dependency on the minimum package version that you want to support.

// bar/package.json
{
  "peerDependencies": {
    "foo": "^0.2.0" // ranges allowed: ~ or ^ for correctness or >= for the lazy 
  },
  "devDependencies": {
    "foo": "0.2.0" // no ranges allowed, no updates allowed without updating peerDependency range
  }
}

I think this is the only way to depend on your own packages in a way that ensures you actually support the versions in the peerdependencies range.

  • assuming you do some amount of integration testing between bar and foo@0.2.0
  • assuming you follow semver correctly (no breaking changes between foo 0.2.0 and 0.7.8)

@ljharb
Copy link

ljharb commented Aug 7, 2018

@StevenLiekens even that won’t protect you against bugs in later versions in the range; you’d have to test against multiple versions. I usually opt for either “earliest, and latest”, or “every minor” - so to make sure npm ls always passes, the dev dep range needs to be identical to the peer dep range.

@sliekens
Copy link

sliekens commented Aug 7, 2018

I'd argue that introducing a bug is a breaking change so it breaks my second assumption. Addings bugs should always be semver-major. 😄

@ljharb
Copy link

ljharb commented Aug 7, 2018

If it’s “added” then it’s a breaking change; bugs are unintentional and can be fixed.

@sliekens
Copy link

sliekens commented Aug 7, 2018

even that won’t protect you against bugs in later versions in the range

I think that's acceptable exactly because it happens unintentionally and can be fixed with a patch-release of the depended-on package without changing the dependent package.

@ljharb
Copy link

ljharb commented Aug 7, 2018

Sure, but then you’ll be artificially insulating yourself from bugs that affect your consumers.

Either way this is getting off topic for the thread :-)

@nolazybits
Copy link

Hello all,

I'm comming here as I started testing publishing and got surprised to see the peerDependencies not updating when publishing with those flags

lerna publish --no-verify-access --no-verify-registry --conventional-commits --exact --registry http://host.docker.internal:4873 prerelease

I would expect Lerna to check the peerDependencies and if one is managed by the monorepo to modify the version following the existence or not of the --exact flag (so hard version or not)

Any idea how to do so atm?
I am using conventional-commit so version is generated automatically by Lerna. How can I specify the peedDependency in advance to match my package version?

@loganfsmyth
Copy link
Contributor

@nolazybits The hard part is that because peerDependencies are part of a module's public API, changes to them can sometimes qualify as breaking changes. The only non-breaking peerDependency change would be to expand the range without removing anything that would match an existing version. Plus, since peerDependencies are usually already ranges, it's not obvious that Lerna could do much.

From my point of view, the only way Lerna could really do it automatically without introducing a breaking change is if you were bumping the major version every single time you made a release, in which case Lerna could change ^56.0.0 to ``^57.0.0` or something like that.

@lock
Copy link

lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants