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

Improved calculating pre-release versions that start with the same base name #1064

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Feb 6, 2025

Suggested merge commit message (convention)

Other (release-tools): Exported the getDateIdentifier() util that converts the current date into YYYYMMDD format.

Other (release-tools): Improved the getLastPreRelease() util to distinguish pre-release versions starting with the same base name. Previously, getLastPreRelease( '0.0.0-nightly' ) matched both 0.0.0-nightly and 0.0.0-nightly-next versions, but now only the first one is matched.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

… package.

Improved `getLastPreRelease()` to distinguish pre-release tags with different names but starting with the same base name.
@coveralls
Copy link

coveralls commented Feb 6, 2025

Coverage Status

coverage: 87.984% (+0.009%) from 87.975%
when pulling dc597e1 on cc/7040
into b70aa02 on master.

@pomek pomek requested review from pomek and filipsobol February 7, 2025 07:53
.filter( version => version.startsWith( releaseIdentifier ) )
.filter( version => {
const optionalDateIdentifier = '(-[0-9]{8})?';
const optionalSequenceNumber = '(.[0-9]+)?';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const optionalSequenceNumber = '(.[0-9]+)?';
const optionalSequenceNumber = '(\.[0-9]+)?';

Match literal .

const optionalSequenceNumber = '(.[0-9]+)?';
const versionRegExp = new RegExp( `^${ releaseIdentifier }${ optionalDateIdentifier }${ optionalSequenceNumber }$` );

return versionRegExp.test( version );
Copy link
Member

@filipsobol filipsobol Feb 7, 2025

Choose a reason for hiding this comment

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

Just a random idea, but maybe we should separate all bits that are important to us by . instead of a mix of . and -?

Given this example:

0.0.0-nightly-next-20230615.0
                  ^        ^

This version number consists of:

MAJOR.MINOR.PATCH-channel_and_date.version

However, according to spec, each identifier in the pre-release portion (after first -) should be dot-separated, so the correct version should be:

0.0.0-nightly-next.20230615.0
                  ^ `.` instead of `-`

This version number consists of:

MAJOR.MINOR.PATCH-channel.date.version

With this approach, we could use semver (which we already have) to parse version numbers instead of using Regexp.

import semver from 'semver';

const data = semver.parse( '0.0.0-nightly-next.20230615.0' );

/**
 * data.raw = '0.0.0-nightly-next.20230615'
 * data.major = 0
 * data.minor = 0
 * data.patch = 0
 * data.prerelease = [ 'nightly-next', '20230615', 0 ]
 */

If this is the only place we do it, then it shouldn't be an issue, but if we have more such places, then relying on official npm tooling would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, due to the fact we are separating nightly-YYYYMMDD with - instead of ., we can't use semver.prerelease() to parse the version, so currently we are doing it manually with regexp.

So question is do we accept to change the versioning for all nightly releases:

  • 0.0.0-nightly-YYYYMMDD.X -> 0.0.0-nightly.YYYYMMDD.X
  • 0.0.0-nightly-next-YYYYMMDD.X -> 0.0.0-nightly.next.YYYYMMDD.X

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with it, but I wonder if it's a breaking change. Or, whether it should be marked as.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants