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

fix: incorrect results from diff sometimes with prerelease versions #546

Merged
merged 7 commits into from
Apr 13, 2023
76 changes: 47 additions & 29 deletions functions/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,54 @@ const parse = require('./parse')
const diff = (version1, version2) => {
const v1 = parse(version1)
const v2 = parse(version2)
if (v1.compare(v2) === 0) {
const comparison = v1.compare(v2)

if (comparison === 0) {
return null
} else {
const hasPre = v1.prerelease.length || v2.prerelease.length
const prefix = hasPre ? 'pre' : ''
const defaultResult = hasPre ? 'prerelease' : ''

if (v1.major !== v2.major) {
return prefix + 'major'
}
if (v1.minor !== v2.minor) {
return prefix + 'minor'
}

if (v1.patch !== v2.patch) {
return prefix + 'patch'
}

if (!v1.prerelease.length || !v2.prerelease.length) {
if (v1.patch) {
return 'patch'
}
if (v1.minor) {
return 'minor'
}
if (v1.major) {
return 'major'
}
}
return defaultResult // may be undefined
}

const v1Higher = comparison > 0
const highVersion = v1Higher ? v1 : v2
const lowVersion = v1Higher ? v2 : v1
const highHasPre = !!highVersion.prerelease.length
const lowHasPre = !!lowVersion.prerelease.length

// add the `pre` prefix if we are going from a stable version
// to a prerelease one
const prefix = highHasPre && !lowHasPre ? 'pre' : ''
wraithgar marked this conversation as resolved.
Show resolved Hide resolved

if (v1.major !== v2.major) {
return prefix + 'major'
}

if (v1.minor !== v2.minor) {
return prefix + 'minor'
}

if (v1.patch !== v2.patch) {
return prefix + 'patch'
}

// at this point we know stable versions match but overall versions are not equal,
// so either they are both prereleases, or the lower version is a prerelease

if (highHasPre) {
// high and low are preleases
return 'prerelease'
}

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'
Comment on lines +40 to +51
Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

special case for inc minor

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.

special case for inc major

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.

Copy link
Contributor Author

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

Copy link
Member

@wraithgar wraithgar Apr 13, 2023

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

}

module.exports = diff
11 changes: 10 additions & 1 deletion test/functions/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Member

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.0.2-1', '0.0.2', 'patch'],
['0.0.2-1', '0.0.3', 'patch'],
Copy link
Member

@wraithgar wraithgar Apr 12, 2023

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.1.0', 'minor'],
Copy link
Member

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', '1.0.0', 'major'],
Copy link
Member

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'

['0.1.0-1', '0.1.0', 'minor'],
['1.0.0-1', '1.0.0', 'major'],
['0.0.0-1', '0.0.0', 'prerelease'],
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
['1.0.1-1', '1.0.1', 'patch'],
Copy link
Member

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.0.0-1', '0.0.0', 'major'],
Copy link
Member

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'

['1.0.0-1', '2.0.0', 'major'],
Copy link
Member

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'

['1.0.0-1', '2.0.0-1', 'major'],
['1.0.0-1', '1.1.0-1', 'minor'],
['1.0.0-1', '1.0.1-1', 'patch'],
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
].forEach((v) => {
const version1 = v[0]
const version2 = v[1]
Expand Down