[BUG] inc premajor, preminor, and prepatch do the wrong thing if the version is already a pre-release. #751
Comments
wraithgar
added
Bug
|
real world use case: npm/template-oss#495 |
|
presumably 2.0.0 + premajor would still jump to 3.0.0-0? |
|
Yes, they all currently do the right thing if the version being incremented is not already a prerelease. > semver.inc('2.0.0', 'major')
'3.0.0'
> semver.inc('3.0.0', 'pre')
'3.0.0-0'
> semver.inc('2.0.0', 'premajor')
'3.0.0-0' |
It seems we also drop the existing preid, unlike |
|
I also want to note that semver inc currently isn't always strict and does "just drop prerelease" Incremented "premajor version" will always drop prerelease. Incremented "preminor version" will drop prerelease if minor change. Incremented "prepatch version" will drop prerelease if patch change. So there is prerelease context aware functionality already incorporated into the existing |
|
Draft PR at #752 |
|
With the draft,
That's confusing. I see these use cases: Using |
|
@mbtools I don't think it's feasible to throw, you need the conditional logic, you missed the case where your version is a |
I would submit that this is a product of design, and unavoidable. Prerelease is not very intuitive and a lot of folks stumble here. The use case here is one of automation. Consider a situation where you know you have a major/minor/patch change to be made, and you also are either in pre-release mode or you aren't. You should be able to tell semver to bump your version with just those two pieces of information. Otherwise it's a LOT more tooling and work to infer what you should do. If I have a piece of software that's on
Consider the original purpose "bump to next release type and drop into pre-release mode". If you're already in the prerelease mode for that type that's effectively just a new pre-release for that same version. ETA:
|
|
I would use the new |
|
How do you make sure the new pre-release is a new version and not the same one as before? |
|
After |
|
I think you'll need to show an example cause that's either the current incorrect behavior or the other incorrect behavior (re-using versions) we're trying to avoid. Let's say we're in Either
Or
We are already in a |
|
To put it another way: we need semver itself to be able to do the correct thing here. We've found that without fail if folks try to hand-roll this logic they get something wrong. We even get it wrong but at least we can centralize the logic and fix it once. If I know:
How do I tell semver, given no other context, how to effect the correct version bump of incrementing my prerelease version only? |
|
There are a couple small mistakes in your comments 😉
Example: 9.1.0
semver -i premajor 9.1.0 // we need a breaking fix
10.0.0-0
semver -i prerelease 10.0.0-0 // wip
10.0.0-1
semver -i prerelease 10.0.0-1 // wip
10.0.0-2
semver -i release 10.0.0-2 // release the fix
10.0.0or 9.1.0
semver -i preminor 9.1.0 // we need a small fix
9.2.0-0
semver -i prerelease 9.2.0-0 // wip
9.2.0-1
semver -i prerelease 9.2.0-1 // wip
9.2.0-2
semver -i release 9.2.0-2 // release the fix
9.2.0 |
It's also preminor! That's the argument we're making here. The The other comment was updated. |
|
in this example How do you know it's only a prerelease bump every time? Where is that logic living? Without this bugfix you have to figure out if you're already in a preminor version, and the change you want to make is no bigger than a minor. It's easy to use the tools as-is if you are a human making these decisions. This is impossible to automate currently without having to write a lot of potentially fragile extra logic that should be in semver itself. |
You are forgetting d) our release process is automated and we need to be able to tell semver what to do from two pieces of information: are we in pre-release mode, and what level change are we making. |
That's contrary to the docs. There's always a version increase:
We can argue whether that's 10.1.0 or 10.2.0 but increasing just the pre-release i.e. staying on the same version is something else. -- I understand the need for automation but I don't see the problem. Let's make it concrete: This repo is on 7.6.3. We get a new PR, a typical "fix". That should trigger If a PR includes "feat" (or whatever criteria is used here), it should trigger If a PR includes "BREAKING", it triggers |
|
Just clarifying, does "preminor" mean "before minor" or "prerelease, minor"? |
|
I'm always referring to the option of the |
|
Right, i just mean conceptually - like which definition are yall using in your head when you see that? |
|
Conceptually, Using the short form "preminor" to describe something like "10.1.0-pre.0" is not very clear and mixing the inc option with the meaning of the version. |
We are asking the question "what does it mean to do this operation on a version that is already in a prerelease for that release type already"? |
|
My interpretation of the doc is that this will always lead to a higher x.y.z version ("bump the version") and is independent of any existing pre-release identifier. My preference is to error if a pre-release id exists, but it works correctly now. |
|
I'm just one semver user and suggest to get some other opinions :-) It would be good to separate |
|
@mbtools maybe it would be helpful to you to take a look at the literal "automated process" function we'd use with release-please https://github.com/npm/template-oss/blob/main/lib/release/semver-versioning-strategy.js#L56-L68 bump(currentVersion, commits) {
const prerelease = this.options.prerelease
const tag = this.options.prereleaseType
const releaseType = parseCommits(commits)
const addPreIfNeeded = prerelease ? `pre${releaseType}` : releaseType
const version = inc(currentVersion.toString(), addPreIfNeeded, tag)
/* istanbul ignore next */
if (!version) {
throw new Error('Could not bump version')
}
return Version.parse(version)
} |
|
If you want to independently add the |
|
Looking at the template code and related test cases was helpful. I agree with the mapping very much 👍 I consolidated it into one statement (see npm/template-oss#497). const nextReleaseType =
!prereleaseMode ? releaseType : // normal release
!isPrerelease ? 'pre' + releaseType : // first prerelease
!isMajor && releaseType === 'major' ? 'premajor' : // minor or patch prerelease and major coming
!isMinor && releaseType === 'minor' ? 'preminor' : // patch prerelease and minor coming
'prerelease' // next prereleaseIf I understand @wraithgar correctly, you would like to reduce this to const nextReleaseType =
!prereleaseMode ? releaseType : 'pre' + releaseType and move the other logic into This is tempting but I don't think it matches the documentation for I'm fine with such change but it's breaking and should be documented accordingly. |
|
@mbtools i'm glad that helped and thanks for the rewrite (a lot of ternaries though), the idea is that we'd remove that function and just embed that logic into semver, it's hard to advocate / explain to third party libraries such as
const nextReleaseType =
!prereleaseMode ? releaseType : 'pre' + releaseType yes!
this is why I commented above with #751 (comment)
For instance this doesn't do what it says: (and would be made more literal by a |
That is a major increase. prerelease is not intuitive. |
|
Alright, we agree on what semver should do for npm 😃 The remaining question: Will it break other projects? I think the existing If a project is on Today, this results in With the suggested change, the second breaking change would be folded into the existing |
I also wonder if this will break for anyone. In your example I personally don't see the point never releasing a version I think the difference in thinking is "are we breaking |
|
I'd say you can't avoid making breaking changes in a prerelease :-p iow that every prerelease is potentially breaking version to version. but i agree that if this is only changing semantics for people who are bumping from one prerelease to another, it's going to affect a very small number of people. |
|
Is there a scenario where someone may be branching out different future versions and keeping multiple "futureports" (opposite of a "backport") at the same time and would like to maintain Here's the argument against the proposal, the "pain path"
This person should just change the version manually, they're non-linearly trying to "increment". Within this scenario the v7 branch still should never jump to v8, though... |
|
We use changesets in the Verdaccio project. I found this:
For a breaking change, they use The good news is that they don't use -- release-please interpretation is similar to what is suggested i.e. keep the version, increase the counter. However, their interpretation of the prerelease number is different. |
Is there an existing issue for this?
Current Behavior
From the docs:
If we do these steps in isolation (using the internal, "pre" that does the
down to a prerelease) we get:But if we do
premajorExpected Behavior
premajoret al should "do what it says on the tin", and not jump an extra major/minor/patch.Steps To Reproduce
No response
Environment
semver@7.6.3The text was updated successfully, but these errors were encountered: