Updated CONTRIBUTING.md with how we version packages #4462
Conversation
JedWatson
requested review from
jesstelford,
timleslie,
molomby,
emmatown and
Noviny
|
|
Thinking out loud: @Noviny @mitchellhamilton is there a way we can customise the changeset bot's message that's added to PRs? would be neat to link to this from there, if it's possible. |
|
|
||
| - `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 |
There was a problem hiding this comment.
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.
Isn't that basically what it says? I'm confused.
There was a problem hiding this comment.
Sorry, I meant the first word of each dotpoint:
breakingmeans a breaking change ...feature additionmeans we added a feature ...bug fixmeans a bug has ...
CONTRIBUTING.md
Outdated
|
|
||
| - 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.
I think all 3 of these should have a patch / bug fix release.
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 .gitignore, etc).
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 patch version bump, which signals to consumers of Keystone that they can safely consume the new version, but gives us a way to narrow down the introduction of any bugs by relying on community reports of issues, and by us bisecting releases to see where a regression was introduced.
There was a problem hiding this comment.
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.
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, patch-but-dont-publish (naming is hard).
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 changesets repo. It would need to allow changesets to be created with no-publish-patch, and then the publishing tool would need to consider this, and not publish packages which only have npp changes. Once a patch/minor/major is available for a package, it gets published and all the npp changes get included in the changelog under either the patch or other heading (🤷♂️)
The Version Packages github bot could also capture this in its generated PR, by including a section of "these packages have changes but are not getting published yet". This would give us visibility on packages which perhaps have had 3 months of dep-bumps and 5 internal refactors but are still unpublished 😅
I think this general idea would strike the right balance between "comprehensive changelogs" vs "version churn".
There was a problem hiding this comment.
We do have a hidden changeset type keystone doesn't use called none - currently it doesn't add info to changelogs, but if you made that add line items, that could be a used thing (it's used currently to allow CI to pass when CI requires a changeset but no change is really needed).
I agree with Jess on noting everything, but even the use of none doesn't solve all problems - it slows down 'version churn' but will still have high 'signal to noise' ratio.
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 manypkg upgrade for keeping my keystone packages up-to-date without too much pain, but have no idea how to make that not so extraneous that nobody will think to do that unless I personally tell them.
There was a problem hiding this comment.
Interesting take. Riffing on the patch-but-dont-publish idea, it makes sense to me if there's a note type change, where it's not a "major" / "minor" / "patch" but just a "note to go along with the changelog" whatever that next version is. Then, next time there is a patch (or any other kind) then the change with the note will go out.
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.
It wouldn't be hard to implement, it's pretty much just a question of where would it be configured? (it would def be a file in the repo but where? I feel a bit uneasy about it being in the changesets config unless the changesets cli actually used it) |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/DeP8sUMaQz5e3bDcZ3euFJXnQXtf |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
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. :) |
|
@JedWatson anything preventing this from merging? We can iterate if needed. |
|
@dcousens there were some different ideas on the bit around
... maybe this section can be taken out, I'm not sure it's true, we should just write what we've been doing I'm not very opinionated about where we land here, maybe sync with @mitchellhamilton and make some calls, as long as you're both happy with it we should merge and iterate 👍 |
Inspired by conversation in #4451
I've been meaning to write this up for a while. @timleslie @molomby @Noviny @mitchellhamilton @jesstelford this is my take on what our guidelines should be, for your review. Happy to discuss/change it, but I think it's important we write it down 🙂