Skip to content

feat(version): add --premajor-version-bump option to force patch bumps for non-breaking changes in premajor packages#3876

Merged
fahslaj merged 17 commits intolerna:mainfrom
jchiem:3863-premajor-version-bump-feature
Nov 21, 2023
Merged

feat(version): add --premajor-version-bump option to force patch bumps for non-breaking changes in premajor packages#3876
fahslaj merged 17 commits intolerna:mainfrom
jchiem:3863-premajor-version-bump-feature

Conversation

@jchiem
Copy link
Copy Markdown
Contributor

@jchiem jchiem commented Oct 26, 2023

Description

  • Added premajorVersionBump (--premajor-version-bump) option for lerna version --conventional-commits, can be configured with either "semver" or "node-semver"
  • This functionality controls how premajor version packages are bumped by lerna when using --conventional-commits with lerna version.
  • "semver" is the default and bumps premajor version packages as minor for feat/fix and is aligned with how lerna handles bumps for premajor version packages today.
  • "node-semver" bumps feat/fix updates as patch.

Motivation and Context

Short summary
Add an option to control version bumps for premajor version when lerna version --conventional-commits is used since node-semver documents it as common to handle minor version bumps as breaking changes and npm is configured to not automatically bump premajor version releases to minor when using ^ in package.json. The recommendation from semver is to try to bump the package to 1.0.0 as fast as possible but there does not appear to be a strict guideline that states that you must do this to be compliant with semver.

This PR is based on the feature request I made in #3863 which @fahslaj suggested I make a PR for so long as it remains backwards compatible.

Original proposal from feature request 3863 Any premajor version package with "feature" (according to conventional commits) should be possible to bump with `patch` but there's code in recommend-version.ts (L76-L94)

This states in the comment that this is in accordance to semver, but the only information I found about this in semver is the FAQ suggesting "the easiest way to handle versions for premajor versions", but it does not explicitly state that all version updates should be done as minor version releases.

node-semver documentation states that "Many authors treat a 0.x version as if the x were the major "breaking-change" indicator." . By default npm install results in "^" denoted versions in package.json. The way lerna always bumps all premajor version packages as "minor" forces users reliant on premajor version packages to manually bump packages as minor if the package is in initial release state but no breaking changes were added.

In my team, we have interpreted the semver documentation in the way node-semver handles this, which means that you should be able to provide feature updates through patch releases on premajor version packages, since any version bump for minor for premajor version packages are considered as potentially containing breaking and can't automatically be bumped by npm if ^ is used.

lerna/libs/core/src/lib/conventional-commits/recommend-version.ts

Lines 76 to 94 in 914dd96

// According to semver, major version zero (0.y.z) is for initial
// development. Anything MAY change at any time. The public API
// SHOULD NOT be considered stable. The version 1.0.0 defines
// the (initial stable) public API.
//
// To allow monorepos to use major version zero meaningfully,
// the transition from 0.x to 1.x must be explicitly requested
// by the user. Breaking changes MUST NOT automatically bump
// the major version from 0.x to 1.x.
//
// The usual convention is to use semver-patch bumps for bugfix
// releases and semver-minor for everything else, including
// breaking changes. This matches the behavior of `^` operator
// as implemented by `npm`.
//
if (releaseType === "major") {
releaseType = "minor";
}
}

How Has This Been Tested?

  1. I've updated independent and independent-prerelease fixtures to include an additional package with "0.1.0" version to ensure that there's coverage of premajor version package bumps in the current state before adding my feature.
  2. I've added two additional fixtures for testing the new preMajorVersionBump option that I added in independent and fixed mode.
  3. I've added two additional tests for version-conventional-commits.spec.ts to validate the new feature as well as backwards compatibility. (Not 100% sure if the 2nd test with "--premajor-version-bump semver" being explicitly set is needed though)

Note: I had to update existing tests to support the additional packages with the additional premajor version package in the fixtures. Tests were also updated ensure the default premajorVersionBump argument passed to recommendVersion is accounted for as part of the expect(recommendVersion).toHaveBeenCalledWith test to continue to work since I've added the feature by passing the option to this function.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (change that has absolutely no effect on users)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Additional information

I have so far not done E2E testing nor Local CLI testing, let me know if these must be done in order to approve the PR, it was not super clear if they must be done for the PR to be approved.

Also I'm not entirely sure about the option name (premajorVersionBump) or the values ("semver" / "node-semver") for the option, if you have any suggestions on better names for this then let me know.

@fahslaj
Copy link
Copy Markdown
Contributor

fahslaj commented Oct 26, 2023

Hi @jchiem , thank you for your work on this. It is looking good. A few things:

  • Please run npx nx format:check and then npx nx format:write to identify and fix formatting issues within the files in this PR.
  • Please add an e2e test for this in e2e/version/src. You can create a new file for this case. There should be plenty of examples of other tests in that folder, but feel free to reach out for help. I will also update the Contributing doc to ensure it is more clear that e2e tests are needed for new features.
  • Please update lerna-schema.json with this new option to support autocomplete in lerna.json files
  • Other small things that I'll comment directly on the files in this PR

Once again, thank you for your work on this feature. Hopefully we can get it in soon!

fahslaj
fahslaj requested changes Oct 26, 2023
View reviewed changes
@fahslaj fahslaj changed the title Feature: add support to handle premajor packages with patch bumps feat(version): add --premajor-version-bump option to force patch bumps for non-breaking changes in premajor packages Oct 26, 2023
@fahslaj
Copy link
Copy Markdown
Contributor

fahslaj commented Oct 26, 2023

As for the naming for the option and the values, that's a tricky one. I think it'd be clearer to have the option values be default or force-patch. The default option is what we have today and defers to the changelog-preset to pick either a minor or patch version. The force-patch option is the new one and overrides whatever suggestion was made from the preset and forces a patch version.

I'm open to suggestions and feedback but I'm leaning towards this over 'semver'/'node-semver'.

@jchiem jchiem force-pushed the 3863-premajor-version-bump-feature branch from ef14b22 to 9754d2c Compare October 27, 2023 07:31
@jchiem
Copy link
Copy Markdown
Contributor Author

jchiem commented Oct 27, 2023 •
edited
Loading

As for the naming for the option and the values, that's a tricky one. I think it'd be clearer to have the option values be default or force-patch. The default option is what we have today and defers to the changelog-preset to pick either a minor or patch version. The force-patch option is the new one and overrides whatever suggestion was made from the preset and forces a patch version.

I'm open to suggestions and feedback but I'm leaning towards this over 'semver'/'node-semver'.

Another option for the naming could be: default and non-breaking-patch, since as you wrote for the README.md, the "force-patch" ensures that premajor version releases without breaking changes will be bumped with patch instead?

I've rebased and updated the PR based on your suggestions for the comments that I have resolved, for now I've gone with force-patch as you have proposed and ran the format:check, format:write.

I have not yet added a e2e test, I'll try to figure out where/how I best do this and let you know if I need more help with that.

@jchiem jchiem force-pushed the 3863-premajor-version-bump-feature branch from 9754d2c to 6acf913 Compare October 27, 2023 07:41
@jchiem jchiem requested a review from fahslaj October 27, 2023 08:34
@jchiem jchiem force-pushed the 3863-premajor-version-bump-feature branch from 6acf913 to bef0eb0 Compare October 27, 2023 08:47
@jchiem
Copy link
Copy Markdown
Contributor Author

jchiem commented Oct 27, 2023

So I've tried running the e2e tests locally but I end up getting errors with the authentication, what steps do I need to perform to be able to run the e2e tests?

I've tried with first setting up Local CLI Testing, that one runs fine and the 999.9.9 version is deployed to my localhost:4873 instance as it should, but the localhost:4872 instance that is started with e2e does not work for me, it appears there's a user by default in the .verdaccio/htpasswd but the e2e config doesnt appear to configure auth like in the verdaccio documentation.

I've had the same issue when trying to run the tests in the main branch as well.

Screenshot 2023-10-27 at 13 49 23 Screenshot 2023-10-27 at 13 49 15

@fahslaj
Copy link
Copy Markdown
Contributor

fahslaj commented Oct 27, 2023 •
edited
Loading

So I've tried running the e2e tests locally but I end up getting errors with the authentication, what steps do I need to perform to be able to run the e2e tests?

I've tried with first setting up Local CLI Testing, that one runs fine and the 999.9.9 version is deployed to my localhost:4873 instance as it should, but the localhost:4872 instance that is started with e2e does not work for me, it appears there's a user by default in the .verdaccio/htpasswd but the e2e config doesnt appear to configure auth like in the verdaccio documentation.

I've had the same issue when trying to run the tests in the main branch as well.

A few things you can try:

  • Run npm run build to make sure that the build isn't failing. If it fails, then the built assets won't be found and lerna: command not found will appear like in your first image.
  • Change the port within local-registry.sh to 4872, start the local registry like in Local CLI Testing, and run npm adduser --registry http://localhost:4872.
  • Run npm install and then npx nx reset to ensure packages are up to date and clear any locally cached builds.

@jchiem
Copy link
Copy Markdown
Contributor Author

jchiem commented Oct 28, 2023

So I've tried running the e2e tests locally but I end up getting errors with the authentication, what steps do I need to perform to be able to run the e2e tests?
I've tried with first setting up Local CLI Testing, that one runs fine and the 999.9.9 version is deployed to my localhost:4873 instance as it should, but the localhost:4872 instance that is started with e2e does not work for me, it appears there's a user by default in the .verdaccio/htpasswd but the e2e config doesnt appear to configure auth like in the verdaccio documentation.
I've had the same issue when trying to run the tests in the main branch as well.

A few things you can try:

  • Run npm run build to make sure that the build isn't failing. If it fails, then the built assets won't be found and lerna: command not found will appear like in your first image.
  • Change the port within local-registry.sh to 4872, start the local registry like in Local CLI Testing, and run npm adduser --registry http://localhost:4872.
  • Run npm install and then npx nx reset to ensure packages are up to date and clear any locally cached builds.

I've tried your suggestions out, I managed to change the port for which local-registry runs verdaccio to 4872 by modifying the .verdaccio/config.yml but not directly in the local-registry.sh.

When I had verdaccio running on port 4872 through npm run local-registry start the e2e test command got a bit further but I got a new error saying im missing a pnpm-lock file. I managed to get rid of that error by doing pnpm install before running the e2e test, next up I got a timeout error. This error I managed to get rid of by increasing the timeout duration to 600000 from 60000, but I'm still getting failed tests.

It seems to complain about the pnpm tarball not matching, is there a specific process I ought to take to update the tarball integrity for 999.9.9-e2e.0 to ensure the e2e tests can pass? Do I need to build/update the expected checksums to run e2e?

Sorry for all the questions but I can't seem to get the e2e tests to work following the current info from the CONTIRBUTING.md nor the steps you have suggested.

@fahslaj
Copy link
Copy Markdown
Contributor

fahslaj commented Oct 30, 2023 •
edited
Loading

Hmm, I would suggest resetting any local changes, like if your lerna.json file differs from the one that already exists in git. A pnpm-lock file shouldn't be needed in the lerna repo itself. You can run pnpm store prune to clear out pnpm's cache, which might fix the tarball not matching issue.

If you're on a completely clean working tree, with no prior processes running on port 4872, you should be able to run npm install, then npm run build, then npx nx e2e e2e-version and the tests should run as expected. What error happens when following this flow?

@jchiem
Copy link
Copy Markdown
Contributor Author

jchiem commented Nov 1, 2023 •
edited
Loading

Hmm, I would suggest resetting any local changes, like if your lerna.json file differs from the one that already exists in git. A pnpm-lock file shouldn't be needed in the lerna repo itself. You can run pnpm store prune to clear out pnpm's cache, which might fix the tarball not matching issue.

If you're on a completely clean working tree, with no prior processes running on port 4872, you should be able to run npm install, then npm run build, then npx nx e2e e2e-version and the tests should run as expected. What error happens when following this flow?

Edit: the error i previously had for npm run build resolved itself after restarting my terminal.

Following these steps, I cloned the repository from scratch and ran the steps in main branch.

I'm getting an error with npx nx e2e e2e-version

lerna ERR! ECONNREFUSED request to http://localhost:4872/@lerna%2fchild-process failed, reason: connect ECONNREFUSED ::1:4872

@jchiem
Copy link
Copy Markdown
Contributor Author

jchiem commented Nov 16, 2023

I've added 2 new e2e tests for fixed mode now.

There was a comment stating: // NOTE: In the independent case, lerna started with version 1.0.0 as its assumed baseline (not 0.0.0 as in the fixed mode case) but I couldnt figure out how to adjust this to be 0.1.0 for the new tests so I've opted to not try to create tests for independent versioning for now.

If you have an easy way to adjust the "first version" of packages in e2e independent mode I can add additional tests.

@fahslaj fahslaj merged commit 3b05947 into lerna:main Nov 21, 2023
@fahslaj
Copy link
Copy Markdown
Contributor

fahslaj commented Nov 21, 2023

Thank you very much for your hard work on this @jchiem !

@ghiscoding ghiscoding mentioned this pull request Nov 24, 2023
Merged
9 tasks
@YoutacRandS-VA YoutacRandS-VA mentioned this pull request Jun 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fahslaj fahslaj fahslaj approved these changes

+1 more reviewer

@queenybee39 queenybee39 queenybee39 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jchiem @fahslaj @queenybee39