The way Keystone does semver is not meaningful to package consumers

[thekevinbrown]

Bug report

We had a heated debate about this yesterday on Slack. Changes to the way we version packages are going to effect most contributors, so I wanted to ensure we could have this conversation openly and involve everyone that wants to be involved, hence the GitHub issue instead of just leaving it at the Slack conversation.

Describe the bug

Currently this is happening to users:

1. I have a working Keystone site.
2. I bump a package with a patch update, e.g. 5.0.1 -> 5.0.2
3. Keystone no longer starts or breaks in strange ways.

Expected behaviour

From semver.org:

Given a version number MAJOR.MINOR.PATCH, increment the:

1. MAJOR version when you make incompatible API changes,
2. MINOR version when you add functionality in a backwards compatible manner, and
3. PATCH version when you make backwards compatible bug fixes.

This patch change is not backwards compatible, so should be a major bump.

What?

But Kevin, we do extensive amounts of work to comply with semver!

Explanation

I'm going to create an oversimplified example of why this happens as I think that would be easier to follow, and I'm going to go painfully slow so every step is clear. I have not researched exactly what combination of packages caused this for Chris on Slack yesterday, but please be assured this is biting people regularly, who are reaching out on Slack and in other GitHub issues. This has also happened on the Demo project, and install, run, broken out of the box is not the best introduction to any development tool.

So, for my made up example, let's simplify and say we only have two packages involved in Keystone, @keystonejs/keystone, and @keystonejs/adapter-knex. We publish as follows:

@keystonejs/[email protected]

// index.js
class Keystone {
    add(a, b) { return a + b }
}

module.exports = { Keystone };

Dependencies: None

@keystonejs/[email protected]

// index.js
class AdapterKnex {
    constructor(keystone) {
        this.keystone = keystone;
    }

    go() {
        console.log(this.keystone.add(1, 2));
    }
}

module.exports = { AdapterKnex };

Dependencies: @keystonejs/keystone: ^5.0.0

So I follow the (again, simplified, not real) docs and do the following in my project:

My Project

// index.js
const { Keystone } = require('@keystonejs/keystone');
const { AdapterKnex } = require('@keystonejs/adapter-knex');

const keystone = new Keystone();
const adapter = new AdapterKnex(keystone);
adapter.go();

Dependencies: @keystonejs/keystone: ^5.0.0, @keystonejs/adapter-knex: ^5.0.0

Success

Everything works at this point.

Output

3

Next Version

We decide that we hate the way keystone.add works, and refactor it to destructure the arguments as follows:

@keystonejs/[email protected]

// index.js
class Keystone {
    add({a, b}) { return a + b }
}

module.exports = { Keystone };

Dependencies: None

Now we've made a breaking change, so we dutifully bumped the keystone package to the next major version. However, adapter-knex's API hasn't changed, so we just bump its dependency and call keystone.add the new way, publishing as a patch:

@keystonejs/[email protected]

// index.js
class AdapterKnex {
    constructor(keystone) {
        this.keystone = keystone;
    }

    go() {
        console.log(this.keystone.add({a: 1, b: 2}));
    }
}

module.exports = { AdapterKnex };

Dependencies: @keystonejs/keystone: ^6.0.0

What now?

I've been hard at work on my Keystone site, and it's almost ready to release. Since I'm all about consuming the latest versions, I use Greenkeeper, Dependabot, Snyk or even just npm-check-updates to see if there are any patches I should pull in before I go live. Using npm-check-updates I see this:

@keystonejs/keystone           ^5.0.0  →   ^6.0.0  // This is red
@keystonejs/adapter-knex       ^5.0.0  →   ^5.0.1  // This is green

Since the project has repeatedly assured me its version numbers comply with semver, I know I can pull in just the patch version and not the major. I'm close to live so I don't want to break anything. I update just adapter-knex.

Or even easier

I don't change package.json in My Project at all, and I just npm i or yarn without a lockfile. Because I have ^5.0.0 for adapter-knex, npm dutifully installs the patch update and leaves v6 of keystone out of it.

In either of these cases

Now we have the following situation when running npm ls:

├── @keystonejs/[email protected]
├─┬ @keystonejs/[email protected]
│ ├── @keystonejs/[email protected]

Which, if adapter-knex did its own requires would be fine, but remember My Project is the one that requires both and hooks them up together.

When I start Keystone, I get:

NaN

The [email protected] dependency is not actually used, as adapter-knex does not require('@keystonejs/keystone') itself. I do the require in My Project. When I do that, I get [email protected], and [email protected], then string them together, as per the docs.

I now have an incompatible combo of packages, all from bumping a single package with a patch version. Even worse is that unless I have integration tests, I may not even notice this is broken if it's deep within Keystone and results in data loss on the way to the DB instead of preventing startup. I may deploy this with CI assuming we're all good and only notice that the patch broke Keystone in production.

Required resolution

Either:

• I should be able to bump any single package in isolation with a patch or minor and not break my site,

or

• We could consider using peer dependencies to declare these, as that's actually what they are, and tooling recognises that. The main issue with peer deps though is npm i without a lock file would dutifully bring in the patch and not the major. Then there'd be a console warning that there's an unsatisfied peer dependency of @keystonejs/[email protected]. This is far from perfect, but at least it's something. The current setup just silently breaks because I pulled in a patch.

or

• We could consider doing version checking at runtime that'd make this more apparent. The problem with only doing this and nothing else is it's still the same experience of install patch, then project breaks when you start it. But, in the positives, at least the break is broadcasted for you and isn't subtle and potentially data-destroying.

or

• We need to stop saying we do semver. Because while it may be true that we technically complied with semver from the perspective of each individual package, we're doing all of this extra work so users can confidently pull in updates. If pulling in a patch update can break a user's site, then why are we doing all of this effort to maintain it?

Desired resolution

It'd be nice if it was more clear what was compatible with what, e.g. #2606. I still feel we can use the version number to do that.

Existing opinions

Here's a summation of each person's viewpoint from the Slack chat yesterday. If you feel this is not a fair representation of what was said, please let me know, I'm doing this to save you having to reiterate, not to put words into your mouth:

@Noviny: This is a bug, users should be able to pull in patch and minor changes being confident that their Keystone won't break. Being able to consume a minor or patch update without having to upgrade anything else should be the outcome of any solution, definitely.

@jesstelford: This is working as expected. What you're experiencing is a combination of a configuration issue and the fact that NPM will install two versions side by side. The fact that one installation ended up with multiple versions is, unfortunately, entirely out of our hands. We are following semver. If you feel you can't trust it, then you can't trust it anywhere in the npm/yarn ecosystem, not just in Keystone.

If you want to be sure you can pull in that patch version, either read through the dependencies in package.json of that package and make sure none have changed majors, and if they have, read through all of those package.json files too, then make sure everything is compatible yourself, or install it, then npm ls and look for duplicates. The semver being a patch does not imply that it's safe to pull in the update by itself, it means that that particular package's API has not changed, and it has not.

@MadeByMike: I see the points on both sides, this is a nuanced discussion, let's talk about it more.

Let's keep the conversation going. I'm particularly interested in what @molomby, @timleslie, @Vultraz, @gautamsi and @JedWatson think about this.

[JedWatson]

Hey @thekevinbrown thanks for the comprehensive write-up! I flagged the conversation in Slack yesterday to come back to and haven't read through all the details yet but I think I've got the jist, and this issue is really helpful 👍

Firstly, an acknowledgement that I think this is a real problem for Keystone's users; and unfortunately I think this is the sort of edge-case issue where the current state of dependency management in the node ecosystem fails us. In theory, peerDependencies are designed to somewhat address this, but (afaik) we don't really have a good constraints system for related optional dependencies. So being right technically is not necessarily good for the community, and it's up to us here to work out how to strike that balance.

Note: in my experience peer dependencies are... basically broken for our use case. If anyone thinks they would help, we can have that conversation.

Also a side note, we can maybe mitigate this a bit by reviewing how the packages in Keystone are actually split up - bearing in mind that a common complaint about versions 1-4 was the "single package" approach and the sheer number of dependencies that were installed but given use case {x} didn't need to be. Add in multiple native modules for database adapters, etc. and the problem gets worse so I expect while we can maybe minimise the number of core keystone packages and therefore reduce the chance for incompatibilities in upgrades to crop up, we can't not deal with it.

Onto how I think we can improve things:

Starting with the example of:

@keystonejs/[email protected]
@keystonejs/[email protected]

I do not think either package should have a dependency on the other one (this is currently not technically correct, see my note at the bottom). keystone should be provided an instance of the class adapter-knex provides, but this means keystone has a dependency on a version of adapter-knex which is unmanaged.

Looks like this:

const { Keystone } = require('@keystonejs/keystone');
const { KnexAdapter } = require('@keystonejs/adapter-knex');

const keystone = new Keystone({
  adapter: new KnexAdapter(),
});

Now, adapter-knex effectively has two APIs:

• one, "user-facing" is the API that users are intended to use (e.g the schemaName option)
• two, "user-facing" is the API that keystone internally uses when provided an adapter instance (e.g the connect method, ref the adapter framework)

Really, they're all part of the same "Public API" of the adapter-knex module, but I think it's helpful to think about this distinction.

A side note about how I think about semver

Semver is, more than anything, the way that package authors have to communicate to users how to interpret new versions. Use major to say "the API or some important internal implementation has changed; you probably need to update, or at least carefully verify, your usage of this package when you upgrade". Use minor to say "we added a feature but it's safe to upgrade". Use patch to say "we fixed something, you should upgrade and expect no changes"

So versions are a bit of an art, because what if "correct behaviour" in a project was depending on a bug in a dependency that gets fixed in a patch release? Often, it's up to package authors to make calls like that, and guess at the probable impact of changes. Which encourages more major versions.

But more major (and worse, really frequent major) versions are a liability for users because of the amount of work required to upgrade safely. Version churn can really hurt a project's community.

Ultimately we have to strike a balance and imo it's better to be cautious with changes, but not at the cost of progress. There's no perfect solution, which comes back to why I think of it as a communication mechanism.

Back to the point

Let's say we (breaking) change the "user-facing" API of adapter-knex, but not the "keystone-facing" API. Easy; major release to adapter-knex and we're done.

Outcome:

@keystonejs/[email protected]
@keystonejs/[email protected]

Now, what if we (breaking) change the "keystone-facing" API and not the "user-facing" API of adapter-knex? well, we're still changing some public API so major release to adapter-knex but... we're not done.

We also need (in theory) a patch update to keystone to correct its usage of the new adapter API. Why patch? well, we're fixing an internal; not changing the public API at all. Users can upgrade to the new version without changing their usage.

... except they can't. They can only safely upgrade if they also upgrade the new major version of the adapter-knex package.

So what do we want to communicate here? I think it's this:

@keystonejs/[email protected]
@keystonejs/[email protected]

But how do users know that these two package upgrades should be linked?

One option is to "lock" major version numbers across multiple packages. But I'm disinclined to do that because I think it'll get out of control and is very likely to introduce unnecessary upgrade churn for users. (unless we moved to monthly releases for major changes, but I don't think we're there yet in terms of maturity as a project. Maybe in the future)

To explain how this can go badly:

We do a major mongoose upgrade, without otherwise changing any of the public API of adapter-mongoose. This should absolutely still be a major version. If versions are locked, that also means a new major of keystone. Which means a new major of adapter-knex. If you're depending on our original packages:

@keystonejs/[email protected]
@keystonejs/[email protected]

Pushing a major out to both of them because we upgraded mongoose (which this project is not even using) communicates a lie, and escalates upgrade fatigue by wasting people's time.

So I believe that lock-stepping versions is bad.

What else we can do

Because I don't believe node / npm / The Platform has a solution for this, I think we should solve it ourselves. I thought about adding this really early on, and now kind of regret not doing it.

Let's imagine that packages, alongside their package version, declare an API Compatibility version.

So in the two packages:

@keystonejs/keystone
  [email protected]
  [email protected]
@keystonejs/adapter-knex
  [email protected]
  [email protected]
@keystonejs/adapter-mongoose
  [email protected]
  [email protected]

If we change the "user-facing" API of adapter-knex we simply increment the package version:

@keystonejs/adapter-knex
  [email protected]
  [email protected]

But if we change the "keystone-facing" API (and corresponding usage) we'd increment the api compatibility version as well:

@keystonejs/keystone
  [email protected]
  [email protected]
@keystonejs/adapter-knex
  [email protected]
  [email protected]
@keystonejs/adapter-mongoose
  [email protected]
  [email protected]

(note that since we're changing the adapter API, we need a new major of the mongoose adapter as well)

The API compatibility range would be validated by keystone when you start it, and an incompatible package would throw with a link to the docs and changelog.

We'd have a handful of API compatibility keys, others I can think of include admin, session, app, field, etc.

This is a bit more work for Keystone, but I like a few things about it:

• means we explicitly throw when incompatible versions of
• we have an abstraction between major package versions and which of those versions are otherwise compatible with each other
• we have a clear way of recognising when we change how keystone interacts with any of its dependencies
• changing a compatibility version immediately means a new major package version
• it should be one way, so it remains manageable; keystone says "I work with this version" and other packages say "I am this version"
• the problem (and this solution) is applicable to multiple parts of keystone where packages have implicit dependencies on the API of other packages, e.g, the File field and the @keystonejs/file-adapters package

I haven't thought through exactly how we'd code the validation yet but it's probably a reasonably straight-forward thing to implement cleanly and I think will prevent future pain related to this issue.

---

Regarding technical correctness of the "these packages should not be dependent on each other at all" statement: today, adapter-knex is dependent on keystone because we need the base adapter classes:

// @keystonejs/adapter-knex/lib/adapter-knex.js
const { BaseKeystoneAdapter, BaseListAdapter, BaseFieldAdapter } = require('@keystonejs/keystone');

I think this needs some more thought, maybe the base adapters belong in another package and not keystone core, to clear up the relationship between packages. But I don't think any outcome here changes the rest of my points above so I'm going to call that discussion out of scope for now.

---

Keen to hear thoughts on this, and if there's any enthusiasm to implement an api compatibility interface.

[thekevinbrown]

To sum up my thoughts:

Yup, you're right about the deps here, I didn't mean to imply this exact scenario happened, or this was the order of the deps in real Keystone, just trying to illustrate how it's breaking for people. Might have been better to call it @keystonejs/a and @keystonejs/b.

I think we can fix this entirely by doing what Gatsby does, which has not gotten out of hand even though they have a lot of packages. Their semver is meaningful to package consumers, and they're a monorepo with a lot of contributors.

On peer deps, why not just remove the dependencies that are unused in this case? They're definitely peer deps (so personally I'd put them in peer deps) but if you don't want to do that, why have them in dependencies at all when the dependency is not used? What's the benefit of declaring a dependency with a version, installing it, then not using it?

I get where you're going with adapter-compat, but personally would be opposed to a bolt on new way of expressing breaking changes on top of semver, because even though the tools aren't perfect here, I feel like install patch -> boom is not a good experience, which is what this would result in, no matter how elegantly we can then model the dependencies.

[thekevinbrown]

Ah, a compromise occurred to me:

• Most of this stuff is hard deps, not optional deps. Declare those as peer deps.
• For the optionals, like mongo vs knex, do a runtime check and don't declare those as peer deps at all, because you're right, NPM/Yarn optional peer deps are not a thing.

That gets most of what we want here, no?

[emmatown]

In theory, peerDependencies are designed to somewhat address this, but (afaik) we don't really have a good constraints system for related optional dependencies

NPM/Yarn optional peer deps are not a thing

Optional peerDependencies very much exist now and I think they would make sense here. (https://yarnpkg.com/configuration/manifest#peerDependenciesMeta.optional, npm/cli@a123410)

I think that a lot of things would probably have non-optional peer dependencies on @keystonejs/keystone and some would have optional peer depedencies on @keystonejs/app-admin-ui and etc.

And because of the way Changesets works with peerDependencies, packages would get major bumps when their peer dependencies get major or minor bumps(we need to do a major bump on a peer dependent when the peer dependency gets even a minor because it could introduce a new feature which the peer dependent depends on, this might cause over-eager major bumps but I think that's better than the opposite and we have some ideas around how Changesets could solve that).

[thekevinbrown]

That solution sounds great @mitchellhamilton!

[JedWatson]

I think we can fix this entirely by doing what Gatsby does, which has not gotten out of hand even though they have a lot of packages.

I'm at risk of being off base here and it's getting late in the day to go into details but... from what I understand, Gatsby is sort of a "hub and spokes" many-package architecture, whereas Keystone is more of a constellation architecture where users choose the shape of the constellation.

Long story short (and hard to explain but) I think the reasons it hasn't gotten out of hand for them won't stay true for Keystone.

What I really want is for Keystone to say "I expect a thing that is compatible with the way I expect to use it" -- without knowing what that package is. Keep in mind, someone could make a closed source flat-file database adapter if they wanted to, implement the adapter API, and it works fine.

When keystone starts it asks the adapter "hey do you conform to [email protected]?" and the project is off and running.

A few major versions and some changes later, keystone says "hey do you conform to [email protected]?" and the adapter we don't know about says "nope, I'm [email protected]" and the developer is informed of exactly what's going on, and what to do about it (basically: see the changelog for where the adapter spec changed and implement those changes, then update the compat version in your package and things will work again)

Sure database adapters (which we're talking about throughout these examples) don't sell the value well because, how many of them are there ever going to be?

But the same pattern holds up for stuff like file adapters (which some fields are provided) and even the fields API -- and then we start to see why this is important, because supporting custom fields is very much a first class concern (and value prop) for Keystone.

So when a List gets configured with fields, it should say "hey I'm expecting to work with [email protected], y'all better support that" to all the fields; and if one of them says "ooh nope I'm 42.4.1 good luck with that" because it's a custom field type nobody outside the project has ever heard of, then we've implemented a really good and scalable DX for keystone's extensions.

This is the crux of why, even given ecosystem tooling at the npm/yarn level, I don't think we can dodge implementing this for long in Keystone - and especially as the community/ecosystem grows. Basically we've gotten away with it so far because of the rate of adoption vs. rate of breaking changes for inter-package APIs.

Optional peer dependencies looks great but breaking changes to inter-package APIs !== major package versions, knowing my field package is compatible with keystone > 11 < 17 and app-admin-ui >8 < 10 is going to get really hard to keep track of for package authors, and I worry about that.

However, I'm talking about introducing a whole new thing to Keystone to solve a problem and I like what @mitchellhamilton has suggested as (at least) an interim solution, and it'll work well for packages in the monorepo which is a great step.

I feel like install patch -> boom is not a good experience, which is what this would result in, no matter how elegantly we can then model the dependencies

💯 agree, we should never patch -> boom

Maybe I didn't call out clearly enough that changing a compat version will always mean a major package version.

Another thing I like about explicitly considering and maintaining an internal API compat version is that, while it's a bit more to consider, as developers of the framework we really should be spending time thinking about that.

From my understanding of the release that originally caused this discussion to be happening now, it was missed that "hey if we change package A so it works with the new API of package B", the version of an instance of a class exposed by package B being passed to package A is actually part of package A's public API and that API has been broken, we should release a new major version.

[gautamsi]

I am no expert here and have not been running a big production to share the pain.

from solution side I believe we can get some help from changeset and manypkg. Given that core contributors are common in these two and Keystone it could be best bet.

If changeset can also generate version matrix of monorepo and is captured somewhere, then manypkg can read this and throw error after every yarn or npm postinstall trigger, just like it does today for other consistency. these matrix can be published as @keystonejs/package-matrix which manypkg make use of or a package.json field which points to a web location for manypkg to download and cache.

idea is that changeset for each release creates something like this:

// changed packages
@keystonejs/fields: 5.1.0,
@keystonejs/keystone: 6.0.0,
@keystonejs/adapter-knex: 5.5.4,
@keystonejs/adapter-mongoose: 5.3.0,
// packages which are not changed,
@keystonejs/util: 5.0.4,
....
....
....
....
....
@keystonejs/admin-ui: 7.6.3,

in this case manypkg can see if there is any version in the dependency not complying with the above matrix and throw error/warning.

I am sure not many would like this solution on top of existing tooling of yarn/npm.

[thekevinbrown]

For the record, many of Gatsby's packages are interlinked via internal APIs, just like Keystone. There are methods on the core package that other packages call to configure the system. There are sources that feed transformers, which feed plugins, etc. There's also an implicit API in the graphql schema they share, and a breaking change will affect that whole pipeline. So no, I still don't think it's inherently different. They handle this by having more rigour around when they release majors, and they batch up the breaking changes so users don't have to deal with lots of majors.

I feel like we're immediately talking about tactics with the app-compat stuff, but there's a bigger conversation about what we're even doing that isn't clear to me. As you've mentioned already, the stated goals are conflicting:

• We want to keep major versions to a minimum.
• We don't want to have to batch breaking changes.
• We want to signal to users when there are breaking changes with major versions.

I think you can only actually have any two of those at once, not all three. For example:

• If we don't batch or avoid making breaking changes and are honest with the semver there'll be a lot of releases. The current velocity seems to be about once a month for Keystone, so if that trend continues, by this time next year we'll be looking at fields v21. That's a lot of releases, and personally I'd find that fatiguing to try to keep up with in production.
• If you publish breaking changes with patch updates, you can indeed keep the version numbers low, but obviously people will have even more release fatigue because they'll have to eye up every patch as a potential breaker, and will be reticent to upgrade at all.
• If you batch breaking changes, then you can comply with semver and keep majors lower, at the cost of having to stick to a release schedule.

I feel like I'm asking, "Which of these two are more important than the other?" and I'm hearing, "All three!" But that's exactly like a client that says, "I want it to be good, fast, AND cheap!" I think we should decide which one of these we want to compromise on, because I just don't see a way to get all three at once. My ideal as a package consumer would be to have a release schedule so our sites don't break and change their DB schema on the regular.

@jesstelford told me on Slack:

This is a false dichotomy, and not helpful.

But I genuinely don't see how that's the case. You either break semver, minimise how many breaking releases you do, or you have a big version number. Those are very clearly the options to me, and I'd love a bit more insight from the team if you feel I'm seeing this wrong.

On the tactics, I don't care what Keystone uses internally to version and maintain semver. As long as the project either says it complies with semver (and actually does), or says it doesn't comply with semver, I'm a happy camper. It sounds to me from what you're saying @JedWatson that we're not ready to follow a release schedule, and we do want to maintain semver, ergo we're expecting to get a pretty high version number. Did I understand correctly?

[thekevinbrown]

From my understanding of the release that originally caused this discussion to be happening now, it was missed that "hey if we change package A so it works with the new API of package B", the version of an instance of a class exposed by package B being passed to package A is actually part of package A's public API and that API has been broken, we should release a new major version.

The issue here is not that, it's that there was a change in a peer dependency that is currently marked as a dependency. Because of that, there's no signal to the project that something external to that package needs to change as part of the update, it just updates its internal dependency, (which is irrelevant as it's not used) then boom. Peer dependencies need to be declared as peer dependencies, or even if we bump a major, there's still going to be no indication from tools like dependabot or greenkeeper that this change needs to happen.

Bumping a major is still better than patch or minor here (this particular example was a minor, that in my view should definitely have been a major), but also, it's a peer dependency that isn't indicated as one.

[gautamsi]

I think there should be a beta builds given there are many changes in #2868

If we plan to make some major changes aligned with packages it will be good time to start doing beta or daily build release and take time in major change release.

[thekevinbrown]

@JedWatson Just a heads up that NPM v7 is going to automatically install peer deps: https://github.com/npm/rfcs/blob/latest/accepted/0025-install-peer-deps.md

So it looks like optional peer deps are the way forward here to me.

[stale]

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)

[emmatown]

Given that most things are now in the @keystone-next/keystone package, the other packages have a peer dependency on @keystone-next/keystone making it clear what version of @keystone-next/keystone it needs and we more consistently do majors when things have a chance of breaking for consumers, I think this is now addressed.