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

tjenkinson · 2023-04-11T22:33:24Z

The following inputs previously gave the incorrect result based on my understanding:

0.0.2-1 => 0.0.3 was prepatch when it should be patch, 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 was prerelease when it should be major. I think 0.0.0-1 => 0.0.0 should be consistent with 1.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 was premajor but should be major 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 be patch for these cases?

I.e 1.1.0-pre => 1.1.0 is achievable with npm version patch and npm version minor. I personally would have expected the diff between those to be patch, just like it would be if it was 1.1.1-pre => 1.1.1

`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

functions/diff.js

wraithgar · 2023-04-12T21:59:10Z

Yeah I think the pre* diff results are very counterintuitive. They are trying to leverage the operations that .inc would do, but diff is not currently directional. Your PR attempts to make the comparison directional which is the part that would need to be discussed.

functions/diff.js

test/functions/diff.js

wraithgar · 2023-04-12T22:18:39Z

Gonna kind of spam those test lines w/ comments sorry for the noise but this deserves a good paper trail

wraithgar · 2023-04-12T22:20:52Z

test/functions/diff.js

    ['0.0.2-1', '0.0.2', 'patch'],
+    ['0.0.2-1', '0.0.3', 'patch'],


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'

wraithgar · 2023-04-12T22:21:17Z

test/functions/diff.js

    ['0.0.2-1', '0.0.2', 'patch'],
+    ['0.0.2-1', '0.0.3', 'patch'],
+    ['0.0.2-1', '0.1.0', 'minor'],


> semver.inc('0.0.2-1', 'minor') '0.1.0'

wraithgar · 2023-04-12T22:21:31Z

test/functions/diff.js

    ['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'],


> semver.inc('0.0.2-1', 'major') '1.0.0'

wraithgar · 2023-04-12T22:22:01Z

test/functions/diff.js

@@ -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],


> semver.eq('1.0.0-1', '1.0.0-1') true

wraithgar · 2023-04-12T22:22:45Z

test/functions/diff.js

    ['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'],


> semver.inc('1.0.1-1', 'patch') '1.0.1'

wraithgar · 2023-04-12T22:23:12Z

test/functions/diff.js

    ['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'],


> semver.inc('0.0.0-1', 'major') '0.0.0'

wraithgar · 2023-04-12T22:24:07Z

test/functions/diff.js

-    ['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'],


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'

test/functions/diff.js

wraithgar · 2023-04-12T22:28:23Z

1.0.0-1 => 2.0.0-1 was premajor but should be major because the biggest change is the major version. Fixed with third commit.

This still seems like premajor as per my comment on the test fixture:

> semver.inc('1.0.0', 'premajor')
'2.0.0-0'

tjenkinson · 2023-04-12T22:36:41Z

Hmm yeh prerelease to prerelease on a different version is a bit confusing. Maybe prerelease to prerelease should also always be pre* ?

tjenkinson · 2023-04-12T22:40:43Z

Is the definition of pre* meant to be ‘going to a prerelease version’? If that’s the case those 3 tests should be pre*

wraithgar · 2023-04-13T00:34:20Z

When using .inc any directive that begins with pre leaves you in a prerelease version. pre does seem to always indicate that the new version is a prerelease.

tjenkinson · 2023-04-13T00:39:44Z

Cool. Ok I pushed a change to make it do that

tjenkinson · 2023-04-13T00:45:25Z

functions/diff.js

+  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'


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

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.

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

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.

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

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

wraithgar · 2023-04-13T01:50:34Z

test/functions/diff.js

+    ['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'],


> semver.inc('1.0.0-1', 'premajor') '2.0.0-0'

wraithgar · 2023-04-13T01:58:18Z

test/functions/diff.js

+    ['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'],


> semver.inc('1.0.0-1', 'preminor') '1.1.0-0'

wraithgar · 2023-04-13T01:58:40Z

test/functions/diff.js

+    ['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'],


> semver.inc('1.0.0-1', 'prepatch') '1.0.1-0'

wraithgar

Gonna let @nlf review this before landing

wraithgar · 2023-04-13T14:12:38Z

diff.js

const 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

1.0.0
inc: 2.0.0
diff: major
1.0.0-pre
inc: 1.0.0
diff: major
1.2.0
inc: 2.0.0
diff: major
1.2.0-pre
inc: 2.0.0
diff: major
1.2.3
inc: 2.0.0
diff: major
1.2.3-pre
inc: 2.0.0
diff: major
1.2.3-0
inc: 2.0.0
diff: major

premajor

1.0.0
inc: 2.0.0-0
diff: premajor
1.0.0-pre
inc: 2.0.0-0
diff: premajor
1.2.0
inc: 2.0.0-0
diff: premajor
1.2.0-pre
inc: 2.0.0-0
diff: premajor
1.2.3
inc: 2.0.0-0
diff: premajor
1.2.3-pre
inc: 2.0.0-0
diff: premajor
1.2.3-0
inc: 2.0.0-0
diff: premajor

minor

1.0.0
inc: 1.1.0
diff: minor
1.0.0-pre
inc: 1.0.0
diff: major
1.2.0
inc: 1.3.0
diff: minor
1.2.0-pre
inc: 1.2.0
diff: minor
1.2.3
inc: 1.3.0
diff: minor
1.2.3-pre
inc: 1.3.0
diff: minor
1.2.3-0
inc: 1.3.0
diff: minor

preminor

1.0.0
inc: 1.1.0-0
diff: preminor
1.0.0-pre
inc: 1.1.0-0
diff: preminor
1.2.0
inc: 1.3.0-0
diff: preminor
1.2.0-pre
inc: 1.3.0-0
diff: preminor
1.2.3
inc: 1.3.0-0
diff: preminor
1.2.3-pre
inc: 1.3.0-0
diff: preminor
1.2.3-0
inc: 1.3.0-0
diff: preminor

patch

1.0.0
inc: 1.0.1
diff: patch
1.0.0-pre
inc: 1.0.0
diff: major
1.2.0
inc: 1.2.1
diff: patch
1.2.0-pre
inc: 1.2.0
diff: minor
1.2.3
inc: 1.2.4
diff: patch
1.2.3-pre
inc: 1.2.3
diff: patch
1.2.3-0
inc: 1.2.3
diff: patch

prepatch

1.0.0
inc: 1.0.1-0
diff: prepatch
1.0.0-pre
inc: 1.0.1-0
diff: prepatch
1.2.0
inc: 1.2.1-0
diff: prepatch
1.2.0-pre
inc: 1.2.1-0
diff: prepatch
1.2.3
inc: 1.2.4-0
diff: prepatch
1.2.3-pre
inc: 1.2.4-0
diff: prepatch
1.2.3-0
inc: 1.2.4-0
diff: prepatch

prerelease

1.0.0
inc: 1.0.1-0
diff: prepatch
1.0.0-pre
inc: 1.0.0-pre.0
diff: prerelease
1.2.0
inc: 1.2.1-0
diff: prepatch
1.2.0-pre
inc: 1.2.0-pre.0
diff: prerelease
1.2.3
inc: 1.2.4-0
diff: prepatch
1.2.3-pre
inc: 1.2.3-pre.0
diff: prerelease
1.2.3-0
inc: 1.2.3-1
diff: prerelease

wraithgar · 2023-04-13T15:43:32Z

I'm happy to land this now. Any problems that still exist are going to be with inc and not diff, now that we've shown that the results match (prepatch/prerelease subtleties notwithstanding).

nlf

this is so great. thank you @tjenkinson!

tjenkinson · 2023-04-14T12:57:52Z

thanks!

tjenkinson added 3 commits April 11, 2023 11:05

fix: diff giving wrong result when going prerelease => stable

65b34a8

`0.0.2-1` => `0.0.3` should be `patch` not `prepatch`

fix: incorrect diff when going from 0.0.0 prerelease to 0.0.0

891b7da

fix: diff not pre* when stable part of 2 prereleases changes

f8c38e9

I.e. `1.0.0-1` => `2.0.0-1` should be `major` not `premajor`, because the biggest change is the major version

tjenkinson marked this pull request as ready for review April 11, 2023 22:37

tjenkinson requested a review from a team as a code owner April 11, 2023 22:37

tjenkinson requested a review from fritzy April 11, 2023 22:37

fix: refactor to remove level of nesting

2c37998

tjenkinson mentioned this pull request Apr 11, 2023

Bump semver from 7.3.8 to 7.4.0 tjenkinson/gh-action-auto-merge-dependency-updates#576

Merged

wraithgar requested review from wraithgar and removed request for fritzy April 12, 2023 21:47

wraithgar reviewed Apr 12, 2023

View reviewed changes

functions/diff.js Outdated Show resolved Hide resolved

Merge branch 'main' into fix-diff-prerelease

f584c08

wraithgar reviewed Apr 12, 2023

View reviewed changes

functions/diff.js Outdated Show resolved Hide resolved

wraithgar reviewed Apr 12, 2023

View reviewed changes

test/functions/diff.js Show resolved Hide resolved

wraithgar changed the title ~~Fix incorrect result from diff sometimes with prerelease versions~~ fix: incorrect results from diff sometimes with prerelease versions Apr 12, 2023

wraithgar requested a review from nlf April 12, 2023 22:08

add comment about prefix

89ae1fe

wraithgar reviewed Apr 12, 2023

View reviewed changes

test/functions/diff.js Outdated Show resolved Hide resolved

fix: make it pre* from diff if going to a prerelease version

f3fd4db

tjenkinson commented Apr 13, 2023

View reviewed changes

wraithgar reviewed Apr 13, 2023

View reviewed changes

wraithgar mentioned this pull request Apr 13, 2023

fix: obey identifierBase in all prerelease increment situations #550

Closed

wraithgar approved these changes Apr 13, 2023

View reviewed changes

nlf approved these changes Apr 13, 2023

View reviewed changes

wraithgar merged commit fc2f3df into npm:main Apr 13, 2023
24 checks passed

github-actions bot mentioned this pull request Apr 13, 2023

chore: release 7.5.0 #549

Merged

snyk-bot mentioned this pull request May 16, 2023

[Snyk] Upgrade semver from 6.3.0 to 7.5.0 OutSystems/cordova-plugin-add-swift-support#3

Open

ms-bot mentioned this pull request Jun 5, 2023

chore(deps): bump 🧳 semver to ^7.6.0 mobsuccess-devops/pmd-github-action#11

Draft

This was referenced Jul 31, 2023

Bump semver from 7.3.5 to 7.5.2 in /platform/ui ossrs/oryx#107

Closed

Bump semver from 7.3.5 to 7.5.4 in /platform/ui ossrs/oryx#111

Closed

Bump semver from 7.3.5 to 7.5.2 in /ui ossrs/oryx#116

Closed

wraithgar mentioned this pull request Oct 18, 2023

fix: correctly increase version for "pre" versions #643

Closed

wraithgar mentioned this pull request Jan 8, 2024

[BUG] diff prerelease changed between 7.3.8 and 7.5.4 #607

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tjenkinson commented Apr 11, 2023 •

edited

wraithgar commented Apr 12, 2023

wraithgar commented Apr 12, 2023

wraithgar Apr 12, 2023 •

edited

wraithgar Apr 12, 2023

wraithgar Apr 12, 2023

wraithgar Apr 12, 2023

wraithgar Apr 12, 2023

wraithgar Apr 12, 2023

wraithgar Apr 12, 2023

wraithgar commented Apr 12, 2023

tjenkinson commented Apr 12, 2023

tjenkinson commented Apr 12, 2023

wraithgar commented Apr 13, 2023

tjenkinson commented Apr 13, 2023

tjenkinson Apr 13, 2023

wraithgar Apr 13, 2023

tjenkinson Apr 13, 2023

wraithgar Apr 13, 2023 •

edited

tjenkinson Apr 13, 2023

tjenkinson Apr 13, 2023

wraithgar Apr 13, 2023 •

edited

wraithgar Apr 13, 2023

wraithgar Apr 13, 2023

wraithgar left a comment

wraithgar commented Apr 13, 2023

wraithgar commented Apr 13, 2023

nlf left a comment

tjenkinson commented Apr 14, 2023

		['0.0.2-1', '0.0.2', 'patch'],
		['0.0.2-1', '0.0.3', 'patch'],

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

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

Conversation

tjenkinson commented Apr 11, 2023 • edited

wraithgar commented Apr 12, 2023

wraithgar commented Apr 12, 2023

wraithgar Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraithgar commented Apr 12, 2023

tjenkinson commented Apr 12, 2023

tjenkinson commented Apr 12, 2023

wraithgar commented Apr 13, 2023

tjenkinson commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraithgar Apr 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraithgar Apr 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraithgar left a comment

Choose a reason for hiding this comment

wraithgar commented Apr 13, 2023

major

premajor

minor

preminor

patch

prepatch

prerelease

wraithgar commented Apr 13, 2023

nlf left a comment

Choose a reason for hiding this comment

tjenkinson commented Apr 14, 2023

tjenkinson commented Apr 11, 2023 •

edited

wraithgar Apr 12, 2023 •

edited

wraithgar Apr 13, 2023 •

edited

wraithgar Apr 13, 2023 •

edited