node-semver
node-semver copied to clipboard
npm
Allow prereleases to start with dots in loose mode
Fixes #164, adds test coverage, many new tests and slight refactoring for additional coverage.
Coverage increased (+2.1%) to 96.828% when pulling 918f472fec8087e0a3ca387f035738f7ed835411 on GH-164 into d21444a0658224b152ce54965d02dbe0856afb84 on master.
Coverage increased (+2.1%) to 96.828% when pulling 918f472fec8087e0a3ca387f035738f7ed835411 on GH-164 into d21444a0658224b152ce54965d02dbe0856afb84 on master.
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?
@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".
Coverage increased (+2.08%) to 96.727% when pulling c0cf701751a2ab48e711aaa2276011fe855a9314 on GH-164 into 32802c53503f67e5ebfd8c59aadf0fcec5d6a4a1 on master.
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.
Coverage increased (+2.08%) to 96.727% when pulling aa24f08e7d0f5ea1e3b268f9f973207db7dbae99 on GH-164 into 32802c53503f67e5ebfd8c59aadf0fcec5d6a4a1 on master.
Coverage increased (+2.08%) to 96.727% when pulling c082957708db28992234e7021e87fa331d3887ed on GH-164 into 8fff3050401397e99aaf7a118015c0e33884c3f8 on master.
Coverage remained the same at 100.0% when pulling 174477e9474f97af0e5ee620d40a41709c43447b on GH-164 into 226e6dc8eca111964ad95881020ee7d7b2b833a2 on master.
This has been rebased and the commit rewritten to conform to conventional commit syntax.
Is this something we still want?
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.
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
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.
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.
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.
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.