Only bump peer dependents when the peer dependency is out of range - #185

🦋 Changeset is good to go

Latest commit: 5da8125

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

This probably isn't something we should do soon since we should probably batch other breaking changes.

Agree.

Would love @lukebatchelor 's opinion before we merge this change as it impacts atlaskit stuff.

Also, other problem - when a package has a peer dependency with an exact version and the peer dep is patch bumped, should the peer dependent be patch or major bumped?

Doesn't this PR lead to the issue where your peerDependency introduces some new functionality (minor change), it is still in range so you don't bump, then down the track you start consuming that functionality. At this point, your package.json is actually wrong right? Because you haven't bumped the minimum required version that would expose that feature to you, so its possible that a consumer has an older (but still satisfying) peerDep which breaks (I'll refer to this as the inconsistent package.json problem.

Or am I misunderstanding the change?

This does bring up a conversation that has been on our backlog for ages though, that I'll likely create a full issue for us to discuss Soon™.

The current bumping behaviour of automatically bumping the minimum versions of packages when they are released at the same time was added to prevent the inconsistent package.json problem. But it has an unintended side effect of creating "version fragmentation".

That is: it increases the total number of combinations of our packages that are not compatible with eachother, leading to package duplication on the consumer (yarn's upgrade behaviour is very no bueno here).

An example would be:

• Button and modal are both version 1.1.1 and modal has a dep on button of version "^1.0.0"
• A consumer currently has button 1.1.0 and modal 1.1.1
• A feature is released to button (minor change) and consumed in modal at the same time (patch change)
• button 1.2.0 and modal 1.1.2 are released, but modals dep on button is now "^1.2.0" (This is good! it prevents the bug we're talking about!)
• The consumer updates their modal dep to latest (1.2.0)
• The consumers own dep on button does not satisfy modals, so it now pulls in its own copy.

At that scale, its .seriously no big problem, everything working as expected. This falls down when you have hundreds of packages depending on eachother and you need to do a sweeping change on all of them at once.

Not only would we not have hit the inconsistent package.json problem, but we've now created a massive line in the sand where any consumer upgrading one package past that line, now need to upgrade every package above that line, otherwise they'll get massive duplication.

Theres a bit more nuance to this problem that we need to talk about but thought I'd mention it briefly (really Luke? Briefly?) to get you across it.

You're understanding this change correctly. The inconsistent package.json problem is certainly a concern with doing this but also, isn't it already potentially a problem because of our current behavour with non-peer dependencies since technically this case could happen:

• [email protected] depends on pkg-b@^1.0.0
• pkg-b is bumped to 1.1.0
• pkg-a is bumped to 1.1.0 and starts depending on behavior added in [email protected] but a package manager could technically keep [email protected] installed

I feel like the trade off of "if you want Changesets to bump peer dependents on minors, use a tilde" is good because it allows both options, yes of course it increases the version fragmentation problem but idk, I feel like it's okay. ¯\_(ツ)_/¯

Also, other problem - when a package has a peer dependency with an exact version and the peer dep is patch bumped, should the peer dependent be patch or major bumped?

IMHO, in that case, it would be OK to just patch that peer dependent - given "pragmatically correct semver" policy 😉

pkg-a is bumped to 1.1.0 and starts depending on behavior added in [email protected] but a package manager could technically keep [email protected] installed

Not sure if I understand this. When [email protected] could get installed without dragging with itself also [email protected]?

A general note - aren't peer deps somewhat a special case in general? From things I use on a daily basis (or have used) the ones that were peers: React, Babel, TypeScript, Emotion, XState. Not really that many - usually we just deal with regular dependencies. So when one develops a monorepo with a thing used as peer dependency (such as Emotion) they should be aware of the extra risk when it comes to version management around this. Of course, this is a manual~ thing (mental overhead) and thus error-prone but I haven't yet seen many times people failing at this. However - with automated releasing, one's instinct around that might fail more often than when managing things manually.

Going back to how it works now - I don't even think this is an ultimate solution, it just maybe minimizes the risk a little bit, right? The only thing that it actually changes is that one might spot unmet peer dep warning after bumping peer-dependent package - but it's not in any way a solution that really makes things seamless (again - due to nature of peer deps). Most times in the wild when peer dep range changes it's actually a major bump for the peer-dependent package - but there is no way for changesets to automate this reliably. So maybe if it cannot be automated it just shouldn't affect (within minor ranger) other packages at all?

Is there any plan to release this change? It is a blocker for us to adopt changeset in firebase-js-sdk. I opened an issue yesterday, and @Andarist pointed me to this PR. Thanks @Andarist !

Currently changeset is guessing that the required minimal version of my peerDependencies changed or will change, and bump it for me, which is not necessarily true. What makes it unusable for us is that it triggers a major version bump on the dependents, while in firebase major versions are coordinated, we can't major release something just because of changes in peerDependency version, especially when they are still in range. I agree with @mitchellhamilton that if we want to avoid inconsistent package.json problem, we would just use tilde version or exact version instead.

I realized the current behavior could be desirable for some people, so perhaps we can add an option that allows peerDependencies to remain unchanged if it is in range. Something like updatePeerDepsWhenOutofRange: true? This way it is not a breaking change and we can release it sooner. @mitchellhamilton WDYT?

I would be much in favor of releasing this behavior under a flag - so people can actually use and test it, even if it comes as unsafe* or similar. I understand being hesitant to just change the current behavior within the current minor version so adding it as a new option (for now) would make a good compromise for both sides of the argument.

Yeah, 💯, this should exist as an unsafe option.

The way I think this should work is that there should be a onlyUpdatePeerDependentsWhenOutOfRange option in a ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH option in the config.

Some thoughts on this:

When we add an option to this, it's us saying "There's probably a problem in here that needs to be solved but it hasn't been totally figured out yet, here's a solution to it that might be right but also might be wrong, we're not sure yet.".

This addresses the immediate problems that people have by letting people experiment with potential solutions but it also solves a much larger problem.

It doesn't restrict us in changing the implementation of Changesets but kind of more importantly, it doesn't restrict the mental model of what Changesets can do and become in the future because it doesn't force us to release solutions that are incomplete or not totally well thought out which means that instead of having to choose between two bad options in the future(1. Remove a feature which people thought was well-supported and therefore annoy a lot of people. 2. Keep a feature and annoy a lot of people/leave them with bad solutions). It's very important to note here that this does not mean we're saying "we don't care about this problem" instead it means, "we likely care about this problem but we won't restrict ourselves to this particular solution."

A practical thing: We'll likely do lots of majors to internal packages(as in the packages that aren't the cli package) when changing these options because otherwise we would have to use exact ranges everywhere but we won't do majors to the cli package when changing these options because that would create a lot of churn for consumers. Users of the experimental options should pin an exact version of @changesets/cli.

Thanks for the insight! That sounds reasonable to me. Do you have a sense of when you will be able to get to it (___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH )? It is the only blocker that prevents me from using changeset.

Probably not very soon. We'd be happy to accept a PR for it though. (assuming @Noviny has no objections to this which I doubt they will)

#383 @mitchellhamilton Can you please take a look?

Closing this PR because having this behavior by default won't come from this PR