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
fix: incorrect results from diff sometimes with prerelease versions #546
Conversation
`0.0.2-1` => `0.0.3` should be `patch` not `prepatch`
I.e. `1.0.0-1` => `2.0.0-1` should be `major` not `premajor`, because the biggest change is the major version
Yeah I think the |
Gonna kind of spam those test lines w/ comments sorry for the noise but this deserves a good paper trail |
['0.0.2-1', '0.0.2', 'patch'], | ||
['0.0.2-1', '0.0.3', 'patch'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is actually two .inc
patches, but that's still a patch
difference. A good test
> semver.inc('0.0.2-1', 'patch')
'0.0.2'
> semver.inc('0.0.2', 'patch')
'0.0.3'
['0.0.2-1', '0.0.2', 'patch'], | ||
['0.0.2-1', '0.0.3', 'patch'], | ||
['0.0.2-1', '0.1.0', 'minor'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('0.0.2-1', 'minor')
'0.1.0'
['0.0.2-1', '0.0.2', 'patch'], | ||
['0.0.2-1', '0.0.3', 'patch'], | ||
['0.0.2-1', '0.1.0', 'minor'], | ||
['0.0.2-1', '1.0.0', 'major'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('0.0.2-1', 'major')
'1.0.0'
@@ -19,10 +19,19 @@ test('diff versions test', (t) => { | |||
['1.1.0', '1.1.0-pre', 'minor'], | |||
['1.1.0-pre-1', '1.1.0-pre-2', 'prerelease'], | |||
['1.0.0', '1.0.0', null], | |||
['1.0.0-1', '1.0.0-1', null], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.eq('1.0.0-1', '1.0.0-1')
true
['0.1.0-1', '0.1.0', 'minor'], | ||
['1.0.0-1', '1.0.0', 'major'], | ||
['0.0.0-1', '0.0.0', 'prerelease'], | ||
['1.0.1-1', '1.0.1', 'patch'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('1.0.1-1', 'patch')
'1.0.1'
['0.1.0-1', '0.1.0', 'minor'], | ||
['1.0.0-1', '1.0.0', 'major'], | ||
['0.0.0-1', '0.0.0', 'prerelease'], | ||
['1.0.1-1', '1.0.1', 'patch'], | ||
['0.0.0-1', '0.0.0', 'major'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('0.0.0-1', 'major')
'0.0.0'
['0.0.0-1', '0.0.0', 'prerelease'], | ||
['1.0.1-1', '1.0.1', 'patch'], | ||
['0.0.0-1', '0.0.0', 'major'], | ||
['1.0.0-1', '2.0.0', 'major'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this is two major .inc
operations but that's still a major diff
> semver.inc('1.0.0-1', 'major')
'1.0.0'
> semver.inc('1.0.0', 'major')
'2.0.0'
This still seems like premajor as per my comment on the test fixture: > semver.inc('1.0.0', 'premajor')
'2.0.0-0' |
Hmm yeh prerelease to prerelease on a different version is a bit confusing. Maybe prerelease to prerelease should also always be pre* ? |
Is the definition of pre* meant to be ‘going to a prerelease version’? If that’s the case those 3 tests should be pre* |
When using |
Cool. Ok I pushed a change to make it do that |
if (lowVersion.patch) { | ||
// anything higher than a patch bump would result in the wrong version | ||
return 'patch' | ||
} | ||
|
||
if (lowVersion.minor) { | ||
// anything higher than a minor bump would result in the wrong version | ||
return 'minor' | ||
} | ||
|
||
// bumping major/minor/patch all have same result | ||
return 'major' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the last part of the PR description, this would make more sense to me if this was all replaced with return 'patch'
The way npm version
behaves with major and minor bumps when the current version is a prerelease and parts are 0
is a bit confusing to me
Specifically I don't understand why there is a 0 special case here and here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments tell the story there:
// If this is a pre-minor version, bump up to the same minor version. // Otherwise increment minor.
A preminor version is a version whose patch is 0
and has a prerelease identifier. Bumping a minor from that is a matter of dropping the prerelease identifier.
Same as before, only we are looking at minor and patch. A premajor version is a version whose minor and patch are 0
and has a prerelease identifier. Bumping a major from that is a matter of dropping the prerelease identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's a bit strange that you can go from 1.0.1-0
to 1.1.0
with npm version minor
but you can't go from 1.0.0-0
to 1.1.0
because the command works differently for that case.
I still don't really get why 0 is special 🤷. Seems a bit inconsistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does appear inconsistent but that is due to the nature of what 1.0.0-0
is. It's the prerelease for a semver major version. Dropping that prerelease is a major, there is no smaller increment you can make outside of increasing the prerelease identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right yes that makes sense. The thing that's confusing I think is that 0.1.0 to 1.0.0 is concretely major, but I'm not sure going from 1.0.0-0 to 1.0.0 could be classed as a major change, it's just going prerelease to stable. Maybe patch/minor/major doesn't really make sense as a thing when going prerelease to stable 🤷
I think the difference between 1.0.1-0 and 1.0.1 has the same weight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make sense if the result from diff when we're going from prerelease to stable release is meant to represent the diff between the previous stable version and the new one 💡, but that's a step I wouldn't expect the diff to be doing internally
['1.0.1-1', '1.0.1', 'patch'], | ||
['0.0.0-1', '0.0.0', 'major'], | ||
['1.0.0-1', '2.0.0', 'major'], | ||
['1.0.0-1', '2.0.0-1', 'premajor'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('1.0.0-1', 'premajor')
'2.0.0-0'
['0.0.0-1', '0.0.0', 'major'], | ||
['1.0.0-1', '2.0.0', 'major'], | ||
['1.0.0-1', '2.0.0-1', 'premajor'], | ||
['1.0.0-1', '1.1.0-1', 'preminor'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('1.0.0-1', 'preminor')
'1.1.0-0'
['1.0.0-1', '2.0.0', 'major'], | ||
['1.0.0-1', '2.0.0-1', 'premajor'], | ||
['1.0.0-1', '1.1.0-1', 'preminor'], | ||
['1.0.0-1', '1.0.1-1', 'prepatch'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.inc('1.0.0-1', 'prepatch')
'1.0.1-0'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna let @nlf review this before landing
diff.jsconst semver = require('.')
for (const level of ['major', 'premajor', 'minor', 'preminor', 'patch', 'prepatch', 'prerelease']) {
console.log(`### ${level}`)
for (const base of ['1.0.0', '1.0.0-pre', '1.2.0', '1.2.0-pre', '1.2.3', '1.2.3-pre', '1.2.3-0']) {
console.group(`- ${base}`)
const inc = semver.inc(base, level)
const diff = semver.diff(base, inc)
console.log(`inc: ${inc}`)
console.log(`diff: ${diff}`)
console.groupEnd(`- ${base}`)
}
} major
premajor
minor
preminor
patch
prepatch
prerelease
|
I'm happy to land this now. Any problems that still exist are going to be with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is so great. thank you @tjenkinson!
thanks! |
The following inputs previously gave the incorrect result based on my understanding:
0.0.2-1
=>0.0.3
wasprepatch
when it should bepatch
, as it's doing a patch operation that goes from the first to the second. Fixed with first commit.0.0.0-1
=>0.0.0
wasprerelease
when it should bemajor
. I think0.0.0-1
=>0.0.0
should be consistent with1.0.0-1
=>1.0.0
. Fixed with second commit. This makes it more consistent but not sure if it's actually what people would expect. See below.1.0.0-1
=>2.0.0-1
waspremajor
but should bemajor
because the biggest change is the major version. Fixed with third commit.I'm actually not convinced the behaviour for the second point is correct. For any version change where the stable part remains the same and the prerelease gets removed,
npm version patch
would give that result. So should the diff always bepatch
for these cases?I.e
1.1.0-pre
=>1.1.0
is achievable withnpm version patch
andnpm version minor
. I personally would have expected the diff between those to bepatch
, just like it would be if it was1.1.1-pre
=>1.1.1