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

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

Closed
1 task done
foxriver76 opened this issue Aug 5, 2023 · 9 comments
Closed
1 task done

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

foxriver76 opened this issue Aug 5, 2023 · 9 comments
Labels
Bug thing that needs fixing Needs Triage needs an initial review

Comments

@foxriver76
Copy link

foxriver76 commented Aug 5, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

a diff between a non prerelease version like 5.5.0 and a prerelease version on the same patch now creates a major diff while it previously was prerelease

see https://github.com/npm/node-semver/pull/566/files

Expected Behavior

It should be either prerelease or mention the breaking change it the changelog and why you think it should be that way.

Steps To Reproduce

import semverDiff from 'semver/functions/diff';

// is major
console.log(semverDiff('1.0.0-alpha.1', '1.0.0'));

Environment

  • npm:
  • Node: 16/18/20
  • OS: windows/linux and mac (all on CI failing due to this problem)
  • platform:
@foxriver76 foxriver76 added Bug thing that needs fixing Needs Triage needs an initial review labels Aug 5, 2023
@ljharb
Copy link

ljharb commented Aug 5, 2023

1.0.0-alpha.1 comes before 1.0.0, and indeed could have a breaking change between the prerelease and 1.0.0. I'm not sure it makes sense to have the arguments in the order you provided, and it should probably throw?

@foxriver76
Copy link
Author

foxriver76 commented Aug 5, 2023

I'm not sure it makes sense to have the arguments in the order you provided, and it should probably throw?

Yes, the check was vice versa actually, just on mobile - however diff should be symmetric as it just compares to versions without a context not like lt and stuff.

Okay if you are seeing it from that perspective this is true. I saw it from a perspective where the version string actually differs. Just not sure how many occurrences exist, that broke, which interpreted it like me and not only use major to interpret if it contains a breaking change.

But then see it the other way around. TBH imo it is a breaking change, but it is not much effort to adapt our code, so I will do this, as we rely on this.

@raphaelokon
Copy link

raphaelokon commented Jan 8, 2024

This tripped us also, we updated semver from 7.3.8 to 7.5.4 and suddenly our prerelease bumps weren’t incrementing correctly, e.g. instead of 1.2.0-nightly.01.2.0-nightly.1 we got 1.2.0-nightly.01.3.0-nightly.0. Still investigating where this is happening.

So what we observed with diff was:

// semver 7.3.8
diff('1.2.0-nightly.0', '1.2.0') // → 'prerelease'

// semver 7.5.4
diff('1.2.0-nightly.0', '1.2.0') // → 'minor'

@raphaelokon
Copy link

@ljharb Is this a bug then? According to the docs regarding diff

Returns difference between two versions by the release type (major, premajor, minor, preminor, patch, prepatch, or prerelease), or null if the versions are the same.

@ljharb
Copy link

ljharb commented Jan 8, 2024

it looks like a bug, since 1.2.0-anything and 1.2.0 diff by "prerelease"

@wraithgar
Copy link
Member

wraithgar commented Jan 8, 2024

No, the diff between 1.2.0-anything and 1.2.0 is a minor. This is admittedly a confusing part of the spec, and is why the bug existed previous to 7.5.4.

> semver.inc('1.1.9-pre', 'minor')
'1.2.0'
> semver.inc('1.1.9-pre', 'patch')
'1.1.9'

The inc you want is probably prerelease since that is what you're trying to increment.

> semver.inc('1.2.0-nightly.0', 'prerelease')
'1.2.0-nightly.1'

This subtlety is why preminor and premajor exist. Because if you were for instance trying to increment from 1.1.9 into a prerelease, you need to specify if it is the prerelease for a major, minor, or patch (the option pre is another confusing one because it goes back into the prerelease form of the current version, unless it's already in prerelease mode then it will increment the prerelease index as shown above. It's best to use prerelease instead of pre and this is why pre isn't recommended for public use).

> semver.inc('1.1.9', 'premajor')
'2.0.0-0'
> semver.inc('1.1.9', 'preminor')
'1.2.0-0'
> semver.inc('1.1.9', 'prepatch')
'1.1.10-0'
> semver.inc('1.1.9', 'prerelease')
'1.1.10-0'
> semver.inc('1.1.9', 'pre')
'1.1.9-0'

@wraithgar
Copy link
Member

Afaict this is working as expected now, the change was a bugfix that was discussed heavily in #546. diff makes every attempt to reflect the change that inc would make given the same parameter.

@raphaelokon
Copy link

Cheers @wraithgar. Thanks for your detailed answer. I assume that the only way that a diff would yield prerelease is then between two prerelease semver versions … e.g. diff('1.1.9-0','1.1.9-1').

@wraithgar
Copy link
Member

wraithgar commented Jan 8, 2024

Yes, exactly. If they differ by anything more you'll get at the very least a prepatch which is the more specific diff between something like 1.2.9 and 1.2.10-0

> semver.inc('1.2.9', 'prerelease')
'1.2.10-0'
> semver.inc('1.2.9', 'prepatch')
'1.2.10-0'
> semver.diff('1.2.9', '1.2.10-0')
'prepatch'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs an initial review
Projects
None yet
Development

No branches or pull requests

4 participants