process: add release.compareVersion(str) functionality #20055
Conversation
This overloads `release.compareVersion` by also accepting a single string argument to compare the version. It also fixes a former bug by comparing the pre-release tags properly. It also adds a stricter input validation to throw on negative values.
BridgeAR
added
the
semver-minor
nodejs-github-bot
added
the
process
doc/api/process.md
Outdated
| added: REPLACEME | ||
| --> | ||
|
|
||
| Perform a SemVer comparison to the release version. |
There was a problem hiding this comment.
The description should come after the parameter list (yes, this should've also been fixed in the original PR).
There was a problem hiding this comment.
Done (including in the original description).
lib/internal/process.js
Outdated
| throw new ERR_INVALID_ARG_TYPE('tag', 'string', tag); | ||
| process.release.compareVersion = function(major, minor, patch, tag) { | ||
| if (arguments.length === 1 && typeof major === 'string') { | ||
| const [start, ...end] = major.split('-'); |
There was a problem hiding this comment.
Would it be faster to just use a regexp to parse and validate the string?
There was a problem hiding this comment.
I did not benchmark this explicitly but I changed it to a reg exp nevertheless because it is also nicer to read.
The return value was not intuitive. It is now exactly the other way around as it was originally implemented. This also fixes a bug with the pre-release tag where the former implementation detected `1.0.0` to be "younger" than `1.0.0-pre`. Fixes: nodejs#20053
BridgeAR
force-pushed
the
improve-process-release
branch
from
54278f4 to
8800470
Compare
|
I pushed two changes: the first to address comments including some stricter checks and the second to fix #20053. That also fixes a minor bug in the former implementation. |
|
someone was opposed to accepting a string when i did it in my pr, i think it was @vsemozhetbyt |
|
Nope, it was not me) I consider this is above my level of knowledge and experience to make strong suggestions about API design) |
|
That was here #19587 (comment) |
|
Right, and I'm still not convinced it's a good idea unless adding support for strings does not slow down the non-string case and parsing the string has very minimal overhead. Even though this kind of check really only needs to be done once for the lifetime of a process, I have a feeling people may use it post-startup and they may expect it to be fast. |
|
@mscdex since I know that the versions read from the RegExp are valid numbers, I do not explicitly coerce them to numbers and it is therefore not monomorphic and I use implicit type coercion in the actual check for strings. If you want, I can change that so it's monomorphic (all cases besides major) if the user does not supply invalid arguments. |
|
proposing reversion of the process.release additions @ #20062 |
|
Closing as it seems like #20062 is preferred. |
This overloads
release.compareVersionby also accepting a singlestring argument to compare the version. It also fixes a former bug
by comparing the pre-release tags properly.
It also adds a stricter input validation to throw on negative values.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes