-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Updated CONTRIBUTING.md with how we version packages #4462
Changes from 1 commit
251d2fc
41ba9d6
ccf6ec7
b2c8bcf
79cc764
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ Contributions which improve the documentation and test coverage are particularly | |
|
||
### Community Ecosystem | ||
|
||
Keystone makes no assumptions about type of applications it powers. It achieves flexibility through small, highly composable parts that allow you to build a foundation for any type of application. | ||
Keystone makes no assumptions about type of applications it powers. It achieves flexibility through small, highly composable parts that allow you to build a foundation for a broad variety of applications. | ||
|
||
For this reason we might not add features to Keystone if they are prescriptive about: | ||
|
||
|
@@ -78,6 +78,51 @@ In particular, please try to write in the past tense (e.g. "Added a new feature" | |
|
||
Thanks for your help with this. | ||
|
||
### How we version packages | ||
|
||
Keystone follows the semver model of {major}.{minor}.{patch}. Version numbers are the first and most obvious way we have of communicating changes to our users, so it's important we convey consistent meaning with them, and strike a careful balance between releasing often vs. overloading consumers with package updates. | ||
|
||
Generally, versions should be interpreted like: | ||
|
||
- `major` means a breaking change to the public API of a package, and/or a meaningful change to the internal behaviour | ||
- `minor` means we added a feature to the package, which is backwards compatible with the current major version | ||
- `patch` means a bug has been fixed in the package | ||
|
||
If a PR includes any of the above, it needs a changeset so the updated packages get released and versioned correctly. | ||
|
||
Other reasons for versioning packages include: | ||
|
||
- If a dependency is updated, and that dependency's API is exposed, the package exposing the API should be bumped by the same level as the dependency being updated. For example a new major version of mongoose would warrant a new major version of keystone. | ||
- If a dependency is updated, and that dependency is not exposed, it may be important to release a package with the update, for example with security fixes. In that case, the package would be bumped with a `patch` version. | ||
|
||
#### Versioning UI changes | ||
|
||
Front-end packages (the Admin UI, Design System, etc) should always follow the rules above for API changes, but may also warrant a version bump for UI changes without an API change. This is more open to interpretation, but should follow the spirit of the rules above: | ||
|
||
- `major` should be used if we're meaningfully changing how the UI looks or works (think: should we update screenshots on the website? might users need to relearn something they know how to do?) | ||
- `minor` should be used if a new feature is available | ||
- `patch` should be used if something has been tweaked or fixed | ||
|
||
#### Versioning example projects | ||
|
||
Since the example projects don't get published anywhere and don't expose API, it's less obvious when they should be versioned. In this case, think of "someone referring to the example project" as an API consumer, and use the version number to communicate anything they should know. | ||
|
||
- `major` means the example has been meaningfully changed, and the difference would break expectations about how it works | ||
- `minor` means the example has had features added or is significantly improved | ||
- `patch` means something has been fixed | ||
|
||
Minor refactoring, including incorporating changes to Keystone APIs, would not warrant updating the package version. | ||
|
||
#### When we don't publish changes | ||
|
||
Sometimes a PR should not include a changeset, or cause a new version of a package to be published. These cases include: | ||
|
||
- A dependency is bumped by a patch version, and the version is specified with a caret range | ||
- A dependency is bumped by a minor or major version but it has no impact on the package API or behaviour (e.g a feature is added in a minor release, but ignored by Keystone) | ||
- Internal refactoring to improve code clarity or organisation that has no impact on the package API or behaviour | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all 3 of these should have a In fact, I'd go so far as to say every PR must have a changeset, with one (very rare) exception: When the change is to do with configuring this repo itself (eg; altering This ensures that any change is given a version, to avoid accidentally releasing it as part of another version which might introduce orthogonal changes. We then avoid issues with 3rd party deps accidentally introducing breaking changes in a patch release, or inadvertently altering undocumented behaviour we were relying on but had forgotten about, etc. Similarly in the "Internal refactoring" situation; unless we have 100% code & branch coverage with our tests, we can never be 100% certain that a refactor breaks something. To clearly separate changes which may have introduced regressions, we can use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are solid points @jesstelford I'm not sure how to balance these, but my counter is I'm deeply concerned about our signal to noise ratio Fundamentally our version numbers are our best (and often only) line of communication with developers using keystone. Following the logic above, especially with the rate we bump deps in this monorepo, we're publishing many of our package weekly, which has a huge multiple on other people spending time bumping packages, reviewing changelogs to see what's updated, and we need to respect that time. If we firehose package updates out that have no meaningful impact or value, and make our changelogs effectively just our commit history, both become useless and people will either become immune to it, ignore them, or have trouble finding the meaningful signal in the noise. My experience is you only have so much capital with users for this sort of stuff before you burn them out, or it becomes frustrating churn they have to live with (and they love using keystone less for it) So we have a bit of a standoff between the things you raise, vs. being able to make a meaningful decision about "is this worth somebody's time and attention". I can definitely see the benefits in being able to isolate changes, but (a) that comes at the cost of all our users' time and (b) it's imperfect because we generally keep a weekly release cadence so some weeks we're batching many changes anyway, where other weeks we release a bunch of packages because we bumped the patch version of a dep that had a caret range anyway, which I think doesn't respect our users' time and attention. Looking at a recent Next.js release as an example, if they released a patch for each of the changes, as a user I think you'd go insane and people would rage about it. That's... what I'm worried we're doing to our users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's interesting that these two comments came up here, because I was mulling this over this morning before reading this PR (after seeing reference to this in other PRs) and trying to work out the crux of the issue. I am on the same page as Jess, I would like to have all our PRs come through with a changeset, because it makes for very useful changelogs. On the other hand, Jed's concern about version churn is legit. I have a thought for how we can solve for both of these; we introduce a fourth changeset option, This option in a changeset would allow the dev to include an appropriate changeset message, which would eventually make it into the changelog, so that when the package does get published we've captured everything, including changes we think "couldn't possibly cause problems" (famous last words). This option would not trigger an actual release of the package though. From a tooling point of view this would mostly need to be done in the The I think this general idea would strike the right balance between "comprehensive changelogs" vs "version churn". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do have a hidden changeset type keystone doesn't use called I agree with Jess on noting everything, but even the use of I do think blogging a summary of each release is pretty helpful as a way to skim out some of the noise and leave more meaningful logs. aside: I've been personally really helped by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting take. Riffing on the This seems pretty worthwhile at least discussing with the Changesets project, because the goal of being able to land changes with a note but not a release seems pretty legit. |
||
|
||
dcousens marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Generally, these guidelines are in place so that we don't spam consumers with version upgrades that don't provide value. They are subjective however, and not "one size fits all" so if you're not sure whether a change warrants a version bump, ask for advice in the PR. | ||
|
||
### Release Guidelines | ||
|
||
#### How to do a release | ||
|
@@ -247,11 +292,11 @@ Now, for each release we want to backport to, we follow this process: | |
) | ||
``` | ||
|
||
<!-- this was the cd command but we don't have a command to replace the exact bolt part yet cd `bolt ws $PACKAGE_NAME exec -- pwd | grep pwd | sed -e 's/.*pwd[ ]*//'` && \ w | ||
--> | ||
|
||
_NOTE: When prompted for "New version", just hit Enter_ | ||
|
||
<!-- this was the cd command but we don't have a command to replace the exact bolt part yet: cd `bolt ws $PACKAGE_NAME exec -- pwd | grep pwd | sed -e 's/.*pwd[ ]*//'` && \ w | ||
--> | ||
|
||
6. Confirm it was published | ||
|
||
```sh | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer referring to these as
breaking
/feature addition
/bug fix
instead of major, minor, patch. It helps convey the meaning much better imo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that basically what it says? I'm confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the first word of each dotpoint:
breaking
means a breaking change ...feature addition
means we added a feature ...bug fix
means a bug has ...