Skip to content
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

feat: add new bump option "prefix" #101

Merged
merged 7 commits into from
May 13, 2022

Conversation

moroine
Copy link
Contributor

@moroine moroine commented Mar 30, 2022

Use Case:

Let's say we have:

  • 2 packages with the following dependency chain a --> b (a is a deps of b & c)

Here are the package.json definition of a & b in the repository (we use 0.0.0 to force using local versions):

{
  "name": "a",
  "version": "0.0.0",
}
{
  "name": "b",
  "version": "0.0.0",
  "peerDependencies": {
     "a": "^0.0.0"
   }
}

When publishing the new version 1.0.1 the published package.json will be:

{
  "name": "b",
  "version": "1.0.1",
  "peerDependencies": {
     "a": "^1.0.1"
   }
}

@antongolub
Copy link
Collaborator

@moroine,

Could plz clarify why --deps.bump='inherit' does not produce the same effect?

@moroine
Copy link
Contributor Author

moroine commented Mar 30, 2022

Because looks like deps.bump will just check if current version satisfy requirement. Hence it will check if 1.0.1 satisfy^0.0.0. But because it doesn't satisfy, it will bump the version package to 1.0.1 without keeping the ^.

I've added a test to be able to compare differences with inherit strategy. I was originally thinking of using inherit but this would be a breaking change

@moroine
Copy link
Contributor Author

moroine commented Mar 31, 2022

@antongolub what do you think about this?

@antongolub
Copy link
Collaborator

Perhaps, we have an inaccuracy in describing of 'bump.inherit' behavior. Let's look on the test instead:

describe("resolveNextVersion()", () => {
	// prettier-ignore
	const cases = [
		["1.0.0", "1.0.1", undefined, "1.0.1"],
		["1.0.0", "1.0.1", "override", "1.0.1"],

		["*", "1.3.0", "satisfy", "*"],
		["^1.0.0", "1.0.1", "satisfy", "^1.0.0"],
		["^1.2.0", "1.3.0", "satisfy", "^1.2.0"],
		["1.2.x", "1.2.2", "satisfy", "1.2.x"],

		["~1.0.0", "1.1.0", "inherit", "~1.1.0"],
		["1.2.x", "1.2.1", "inherit", "1.2.x"],
		["1.2.x", "1.3.0", "inherit", "1.3.x"],
		["^1.0.0", "2.0.0", "inherit", "^2.0.0"],
		["*", "2.0.0", "inherit", "*"],
		["~1.0", "2.0.0", "inherit", "~2.0"],
		["~2.0", "2.1.0", "inherit", "~2.1"],
	]

If ["~1.0", "2.0.0", "inherit", "~2.0"] works why ["^0.0.0", "1.0.0", "inherit", "^1.0.0"] does not — that's what i'm trying to figure out.

@moroine
Copy link
Contributor Author

moroine commented Apr 14, 2022

@antongolub did you got time to have a look at it?

@dbouwman
Copy link

@antongolub we are also running into a similar issue where the ^ is being removed.

I my test project, I ran changes that should have hit this test case (in dependencies and peerDependencies)

["^1.0.0", "2.0.0", "inherit", "^2.0.0"],

except that it actually inserted "2.0.0" when running in a real release.

@antongolub
Copy link
Collaborator

@moroine, @dbouwman,

Maybe it's better to introduce override-prefix? So we could also pass ~.

@moroine
Copy link
Contributor Author

moroine commented May 11, 2022

@antongolub this is very problematic. Do you prefer to add this PR or still want to find out if this is a bug.

About override-prefix I think that's a good idea, but what would be the value of deps.bump ?

@antongolub
Copy link
Collaborator

@moroine, there's no bug as I can see now. And your feature is not covered by satisfy strtegy, I was wrong. So I suggest tweaking a bit your PR:

  1. Introduce --deps.prefix = '' | '^' | '~' that will be attached to the next v if --deps.bump === 'override'.
  2. Refactor resolveReleaseType, updateManifestDeps

I've landed your proposal on our experimental fork: https://github.com/qiwi/multi-semantic-release/pull/70/files

@moroine
Copy link
Contributor Author

moroine commented May 11, 2022

@antongolub your changes are excellent, I've also used your descriptions 😄

What are the differences with qiwi/multi-semantic-release? Does it respect the dependencies order, but maximize parallelization?

@antongolub
Copy link
Collaborator

image

The main diff: @qiwi fork replaces concurrent event-driven flow with toposort-based queues. It's definitely slower, but it's easier to debug our proprietary plugins.

package.json Outdated
@@ -5,7 +5,8 @@
"license": "0BSD",
"engines": {
"node": ">=10.18",
"yarn": ">=1.0.0"
"yarn": ">=1.0.0",
"npm": "please-use-yarn"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... npm v7+ works with monorepos as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the repository is using yarn.lock and npm will ignore yarn.lock and create package-lock.json; hence this will remove the advantage of having lock file

Copy link
Collaborator

@antongolub antongolub May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is clear, but I've never seen this kind of restriction anywhere else) Maybe it would be enough just to add package-lock.json to gitignore instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antongolub I revert this change

@moroine moroine requested a review from antongolub May 12, 2022 09:29
@antongolub
Copy link
Collaborator

antongolub commented May 12, 2022

@moroine, LGTM, I'm ready to merge. But could you check what happens if:

if (scope[dependency.name]) {
	scope[dependency.name] = resolveNextVersion(dependency._lastRelease?.version || '0.0.0', dependency._nextRelease?.version, bumpStrategy, prefix);
} 

I think it would be better if only resolveNextVersion implted prefix handling logic.

@antongolub
Copy link
Collaborator

antongolub commented May 12, 2022

@moroine, this works:

const { isObject, isEqual, transform, get } = require("lodash");
//...
if (scope[dependency.name]) {
    scope[dependency.name] = resolveNextVersion(
	get(dependency, "_lastRelease.version", "0.0.0"),
	release.version,
	bumpStrategy,
	prefix
    );
}

@antongolub antongolub merged commit 001e344 into dhoulb:master May 13, 2022
@antongolub antongolub changed the title feat: add new bump-strategy "override-carret" feat: add new bump option "prefix" May 13, 2022
@antongolub
Copy link
Collaborator

@moroine, finally merged.

Thanks a lot for your improvements and for your patience! The new feature will be available in v2.13.0.

@antongolub
Copy link
Collaborator

🎉 This PR is included in version 2.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants