feat(conventional-commits): Add conventional prerelease/graduation #1991
Conversation
erquhart
force-pushed
the
conventional-commits-prerelease
branch
4 times, most recently
from
c78d9a0
to
eeaabaf
Compare
erquhart
changed the title
erquhart
force-pushed
the
conventional-commits-prerelease
branch
5 times, most recently
from
04bcc3b
to
2ba19b5
Compare
|
You sir, have made my day. Can’t wait to try this out. Wonderful job! |
erquhart
force-pushed
the
conventional-commits-prerelease
branch
2 times, most recently
from
35c42bd
to
72f922a
Compare
| @@ -346,6 +377,7 @@ class VersionCommand extends Command { | |||
| changelogPreset, | |||
| rootPath, | |||
| tagPrefix: this.tagPrefix, | |||
| prereleaseId: getPrereleaseId(node), | |||
There was a problem hiding this comment.
Logic determining whether to prerelease stable packages (shouldPrerelease) or graduate prereleasePackages (shouldGraduate) is encapsulated here so that ConventionalCommitUtilties.recommendVersion is only reacting to the presence or absence of prereleaseId.
| @@ -35,8 +49,15 @@ function recommendVersion(pkg, type, { changelogPreset, rootPath, tagPrefix }) { | |||
| // we still need to bump _something_ because lerna saw a change here | |||
| const releaseType = data.releaseType || "patch"; | |||
|
|
|||
| log.verbose(type, "increment %s by %s", pkg.version, releaseType); | |||
| resolve(semver.inc(pkg.version, releaseType)); | |||
| if (prereleaseId) { | |||
There was a problem hiding this comment.
prereleaseId both activates prerelease recommendations and provides the prerelease ID to be used. This function doesn't know why it's prereleasing, just whether to do it or not.
| let candidates; | ||
| // --conventional-commits --conventional-graduate | ||
| if (useConventionalGraduate) { | ||
| if (forced.has("*")) { |
There was a problem hiding this comment.
If --conventional-graduate is different from --force-publish in that it only impacts packages that are in prerelease and never affects packages that are not in any way. This is why we don't set all packages as candidates here as we do with --force-publish.
|
|
||
| module.exports = collectPackages; | ||
|
|
||
| function collectPackages(packages, { isCandidate = () => true, onInclude } = {}) { |
There was a problem hiding this comment.
This felt like a smart abstraction as prerelease functionality introduced a need to collect dependents from outside of collect-updates. Tests were added for the new function.
|
|
||
| module.exports = getPackagesForOption; | ||
|
|
||
| function getPackagesForOption(option) { |
There was a problem hiding this comment.
Changed getForcedPackages to this more generic function for use with options besided --force-publish.
| const { forcePublish, conventionalCommits, conventionalGraduate } = this.options; | ||
| const checkUncommittedOnly = forcePublish || (conventionalCommits && conventionalGraduate); | ||
| const check = checkUncommittedOnly ? checkWorkingTree.throwIfUncommitted : checkWorkingTree; | ||
| tasks.unshift(() => check(this.execOpts)); |
There was a problem hiding this comment.
These four lines are the fix for #1675.
There was a problem hiding this comment.
hrm, ok, this is confusing when conflated with the rest of the logic. I'd prefer this was in a separate PR for clarity.
There was a problem hiding this comment.
This PR isn't complete without these four lines - graduation of prereleases is facilitated by passing --conventional-graduate, and that fails without this fix. It just so happens that this same issue is also a known bug for --force-publish.
|
|
|
@erquhart This is exciting, I will have time to review soon! |
|
Update: still working great in production. We now have a workflow of prereleasing to Recent dry run showing graduations after a few rounds of prerelease: Latest commit (Lerna release) Dry run of Note the differences in bump level, some patch, some minor, varying prerelease increment, all graduated to the proper version. |
commands/publish/index.js
Outdated
| const distTag = getDistTag(pkg.get("publishConfig")); | ||
| const preDistTag = this.getPreDistTag(pkg); | ||
| const distTag = preDistTag || getDistTag(pkg.get("publishConfig")); | ||
| opts.tag = preDistTag || opts.tag; |
There was a problem hiding this comment.
We really shouldn't be mutating the outer opts.tag inside the mapper. The only value that matters here is distTag, the parameter passed to npmDistTag.add(), opts.tag should never be considered when the underlying code executes (it's always overwritten by the parameter here).
There was a problem hiding this comment.
Good catch, I wasn't understanding that the config snapshot should continue to show the dist-tag and ignore pre-dist-tag (as they can be used together).
commands/publish/index.js
Outdated
| if (!this.options.preDistTag) { | ||
| return; | ||
| } | ||
| const isPrerelease = (semver.prerelease(pkg.version) || []).shift(); |
There was a problem hiding this comment.
This logic is lifted from here, I'd rather not duplicate it.
| const isPrerelease = (semver.prerelease(pkg.version) || []).shift(); | |
| const isPrerelease = this.packageGraph.get(pkg.name).prereleaseId; |
There was a problem hiding this comment.
The problem is that prereleaseId can now change, but we're currently setting it as a static value. I was considering addressing this but wanted to limit PR scope creep wherever possible. I'll update packageGraph to use a getter for prereleaseId.
There was a problem hiding this comment.
Actually, we're working with a Package, not a PackageGraphNode - the version isn't up to date in the graph when we need to get the preDistTag, so have to determine prereleaseId from the Package instances in the mapper.
Considering that this logic was already duplicated here (and in the related test), I'll just separate out the logic itself. It should probably be tested directly anyway, critical little bit of logic here.
| @@ -48,6 +48,9 @@ expect.addSnapshotSerializer(require("@lerna-test/serialize-git-sha")); | |||
|
|
|||
| describe("VersionCommand", () => { | |||
| describe("normal mode", () => { | |||
| beforeEach(() => { | |||
| checkWorkingTree.mockReset(); | |||
There was a problem hiding this comment.
Why is this necessary? We are already clearing mocks after every test.
There was a problem hiding this comment.
Resetting removes mock implementations (clearing doesn't). This shouldn't have been necessary since mockImplementationOnce is being used, a second look revealed that I shouldn't be mocking an implementation for an error that is not expected. Fixed.
| const testDir = await initFixture("normal"); | ||
| await lernaVersion(testDir)("--force-publish"); | ||
|
|
||
| expect.assertions(0); |
There was a problem hiding this comment.
I would prefer this take make an active assertion, such as expect().not.toThrow(). Also, unclear why this test is in this PR?
There was a problem hiding this comment.
Regarding why this test is here, it's because --force-publish happened to be fixed here. Will address assertion style.
There was a problem hiding this comment.
Making sure this particular call doesn't throw is just a bad approach in general. Going to test for a positive outcome instead.
| const { forcePublish, conventionalCommits, conventionalGraduate } = this.options; | ||
| const checkUncommittedOnly = forcePublish || (conventionalCommits && conventionalGraduate); | ||
| const check = checkUncommittedOnly ? checkWorkingTree.throwIfUncommitted : checkWorkingTree; | ||
| tasks.unshift(() => check(this.execOpts)); |
There was a problem hiding this comment.
hrm, ok, this is confusing when conflated with the rest of the logic. I'd prefer this was in a separate PR for clarity.
|
@evocateur I added commits to close the first round of review, replied to each of your actionable comments with commit links. I know the difficulty of reviewing a large changeset like this, especially from a new contributor. Hope it helps! |
|
|
||
| function prereleaseIdFromVersion(version) { | ||
| return (semver.prerelease(version) || []).shift(); | ||
| } |
There was a problem hiding this comment.
Yes this feels a bit like overkill, but I really couldn't find a better path that fits existing code org patterns in the repo. This (ridiculously simple) logic is reused in different verticals across the codebase, like core and command, so it couldn't be dropped in an existing common lib. If you're up for adding a lib directory at the top level for little bits like this, I'm happy to drop this commit and add one that does that instead.
erquhart
force-pushed
the
conventional-commits-prerelease
branch
from
a9ba5df
to
4fa9017
Compare
|
|
||
| module.exports = collectPackages; | ||
|
|
||
| function collectPackages(packages, { isCandidate = () => true, onInclude } = {}) { |
|
|
||
| module.exports = getPackagesForOption; | ||
|
|
||
| function getPackagesForOption(option) { |
|
|
||
| function prereleaseIdFromVersion(version) { | ||
| return (semver.prerelease(version) || []).shift(); | ||
| } |
package-lock.json
Outdated
| "version": "7.2.2", | ||
| "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.2.2.tgz", | ||
| "integrity": "sha512-59vB0RWt09cAct5EIe58+NzGP4TFSD3Bz//2/ELy3ZeTeKF6VTD1AXlH8BGGbCX0PuobZBsIzO7IAI9PH67eKw==", | ||
| "version": "7.3.4", |
There was a problem hiding this comment.
I'm not entirely sure why there are so many diffs in the lockfile, but if it isn't conflicting, I'm fine with it.
| @@ -154,6 +155,14 @@ lerna publish --canary --preid next | |||
| When run with this flag, `lerna publish --canary` will increment `premajor`, `preminor`, `prepatch`, or `prerelease` semver | |||
| bumps using the specified [prerelease identifier](http://semver.org/#spec-item-9). | |||
|
|
|||
| ### `--pre-dist-tag <tag>` | |||
There was a problem hiding this comment.
I don't see any of these flags documented in the yargs command builder for publish: https://github.com/lerna/lerna/blob/master/commands/publish/command.js
I suppose it's not vital, since yargs seems perfectly happy to consume them implicitly...
evocateur
force-pushed
the
conventional-commits-prerelease
branch
from
4fa9017
to
05e0fd7
Compare
|
That was a fun rebase (my own fault) |
|
Thank for getting it in! |
|
Hi, I believe the behavior of If you guys can tell me if I'm missing something in the way semver works, thanks 👍 |
|
Conventional commits didn't really have any logical or intentional behavior for prereleases, so it's fair to say that using But to your immediate point, yes, to duplicate the result of |
|
Wait, |
|
No, |
|
Thank you @erquhart it makes sense to me now! 😄 |
|
does anyone have this working in CI? I keep getting a prompts even with |
|
I've used in CI in the past without issue, been doing it manually the last few months, though. Maybe something changed? |
Description
Provides
--conventional-commitsrecommended bumps for prerelease packages. Ensures that the current prerelease version, as well as the eventual graduated version, are semver compliant based on the commit messages of contained changes. When additional prereleases occur and the current prerelease base version is already adequate, the numeric prerelease identifier is incremented.Prerelease bump logic is best illustrated in related tests:
lerna/core/conventional-commits/__tests__/conventional-commits.test.js
Lines 262 to 285 in 72f922a
Updates
--conventional-commitsto keep prereleased packages in prerelease and stable packages out of prerelease, but still providing semver bumps as expected.Flags for use with
--conventional-commits:--conventional-prereleaseReleases all unreleased changes to pre(patch/minor/major/release) by prefixing the recommendation from
conventional-changelog. Optionally accepts specific packages, in which case unspecified packages with changes present will be processed as they normally would using--conventional-commits.--conventional-graduateGraduates all prereleased packages, whether the current HEAD has been released or not. Similar to
--force-publish, but ignores non-prerelease packages (unless they depend on a package that is being graduated). If unreleased changes are present, and the base version is inadequate for those changes, eg. features in a prepatch, the resulting package version(s) will reflect the recommended bump fromconventional-commits. Optionally accepts specific packages, in which case unspecified packages will be processed as they normally would using--conventional-commits.Flags for use with or without
--conventional-commits:--pre-dist-tagWorks identically to
--dist-tag, except only applies for packages being released with a prerelease version. Necessary for proper npm dist tagging with independent versioning, where prerelease and stable versions may be released at once, and require different tags. Can be combined with--dist-tag.Motivation and Context
Fixes #1433 by providing a predictable, CI compatible approach to prerelease publishing for projects using conventional commits, including those using independent or fixed versioning. My immediate context is a desire to publish
betareleases frommasterevery time a new PR is merged.Fixes #1675 by using
checkWorkingTree.uncommittedChangesdirectly in theversioncommand script when--force-publish(or--conventional-graduate) is set.How Has This Been Tested?
netlify-cmsusing a published testing fork of this PR (@erquhart/lerna).Types of changes
Checklist: