fix: allow . to prefix prerelease in loose mode

[isaacs]

BREAKING CHANGE: prerelease tag requires a - if it doesn't start with
a non-numeric value. If it does start with a non-numeric value the -
is optional and a . can be used instead. This means that 1.2.34.5
is no longer a valid version in loose mode (previously it would split
into 1.2.3-4.5 and 1.2.3.4 is invalid (previously it would split
into 1.2.3-4.

Allowing 1.2.3.4 is confusing and weird.

However, 1.2.3.beta should be allowed.

Fixes #164, adds test coverage, many new tests and slight refactoring for additional coverage.

[coveralls]

Coverage increased (+2.1%) to 96.828% when pulling 918f472 on GH-164 into d21444a on master.

[coveralls]

Coverage increased (+2.1%) to 96.828% when pulling 918f472 on GH-164 into d21444a on master.

[othiym23]

To me, 1.2.3.4.5.6 and 1.2.3-4.5.6 are different in kind, and casting the former to SemVer implicitly feels like a stretch, even for loose mode. Having node-semver infer a prerelease tag like this feels like a stretch, given that in different versioning schemes those versions may have completely different intentions. Especially given the current matching rules around prerelease tags, changing the interpretation like this is surprising. What's the use case for this change?

[isaacs]

@othiym23 Yeah, 1.2.3.4 => 1.2.3-4 is kind of a weird side-effect. No one has asked for that.

The use case is in the OP in #164.

Loose parsing allows pr tags to not have the -, so you can have things like 1.2.3foo parsed the same as 1.2.3-foo. Prerelease tags are a series of prerelease identifiers separated by . characters. So, when you have something like 1.2.34.foo, it sees a valid tuple followed by a valid set of prerelease identifiers, and parses as 1.2.3-4.foo, when the intent is clearly 1.2.34-foo.

I guess maybe a workaround would be to allow it to start with a . if the next character is not a number. That'd block 1.2.3.4 but 1.2.34.5 would still be misinterpreted as 1.2.3-4.5.

So, a second step would be to require the - if the first prerelease tag identifier is numeric.

Then the pattern looks something like {tuple pattern}(\-({prids})|(\.?[a-z]{prids}))?, where prids is the pattern saying "1 or more prerelease identifiers".

[coveralls]

Coverage increased (+2.08%) to 96.727% when pulling c0cf701 on GH-164 into 32802c5 on master.

[isaacs]

Modified the prerelease regexp so that it requires a - if it doesn't start with a non-numeric value. If it does start with a non-numeric value, then the - is optional, and a . can also be used instead.

This means that 1.2.34.5 is no longer a valid version in loose mode (before it would split into 1.2.3-4.5) and 1.2.3.4 is no longer a valid version either (as of the rest of this PR, it would split into 1.2.3-4).

Pretty sure that makes this a breaking change.

[coveralls]

Coverage increased (+2.08%) to 96.727% when pulling aa24f08 on GH-164 into 32802c5 on master.

[coveralls]

Coverage increased (+2.08%) to 96.727% when pulling c082957 on GH-164 into 8fff305 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 174477e on GH-164 into 226e6dc on master.

[wraithgar]

This has been rebased and the commit rewritten to conform to conventional commit syntax.

Is this something we still want?

[ljharb]

I'm not sure it is - if this change lands and is published as a semver-major, wouldn't it be really easy to publish a package with a 1.2.3.beta prerelease that breaks everyone on an npm (or other CLI) version that didn't support the format?

If so this is a lot of risk solely to allow 1.2.3.beta instead of 1.2.3-beta or something.

[wraithgar]

npm does not use loose checking when validating semver for publishing. It calls semver.clean which in this PR still returns null (which would cause npm to refuse to publish)

> require('.').clean('1.2.3.beta')
null

[ljharb]

ah, fair enough. in that case, "i guess"? but it seems weird to make breaking changes to loose mode when npm doesn't even use it.

[wraithgar]

I think enough time has passed without this being implemented that it's likely not something anyone needs or is asking for. It also goes against the actual SemVer spec, and drifting further from that spec should only be done if absolutely necessary.

[ljharb]

Agreed. (also, if it's a breaking change, "fix:" seems inaccurate)

[wraithgar]

I agree and it's my one gripe w/ conventional commits, the breaking change prefix isn't its own thing it's a footer or addendum to an existing type. We settled on doing it this way.

[isaacs]

Agreed. (also, if it's a breaking change, "fix:" seems inaccurate)

An ironic thing about semver is that there's rarely any question between minor and patch, or minor and major, but often the question as to whether something should be patch or major is very subtle.

The impetus for this was parsing version strings that come from other non-semver systems. When it was requested, loose mode was used much more often, and much of the registry was still published under the old SemVer 1.0 specification. But enough time has passed without any serious call for this, that I think it's fine to just punt on it, personally. Especially given the cost of a semver major bump to this module.