Skip to content

Confusing process.release.compareVersion API #20053

Description

@BridgeAR

It might only be me but the current implementation is not intuitive for me because I expect the output to be exactly the opposite of what it is. Even though it is not right or wrong either way.

Let's give an example.

The current master version is 10.0.0-pre.

That way I expect 9.0.0 to return -1 because the input value is lower than the current version. Instead, it is 1, because the current version is bigger than the input value. So it depends on what viewpoint the user is in.

For me, it is more intuitive to have the input value being the main important part and not the current version (I hope it is clear what I want to express...).

So I would like to hear from others @nodejs/collaborators how they feel about this (it did not yet land on any release, so there would theoretically still be time to change this without being semver-major).

Activity

  1. added
    metaIssues and PRs related to the general management of the project.
    processIssues and PRs related to the process subsystem.
    on Apr 15, 2018
  2. tniessen commented on Apr 15, 2018

    @tniessen
    Member

    @devsnek originally landed the API with the signum as you described in #19438, which was reverted after I expressed my concern that the signum might be counterintuitive (see #19574 (comment)):

    The signum of the result of compareVersion seems counter-intuitive to me. In most (if not all) frameworks, a comparator returns a negative result if the first argument (or this) is considered to be "before" the second argument, and a positive result if the first argument (or this) is considered to be "after" the second argument (see e.g. java.lang.Comparable for a single-argument comparison). compareVersion contradicts this well-established norm. I would expect compareVersion(10, 0, 0) >= 0 to return whether this release is v10 or newer, which follows the norm and is intuitive, but #19438 implements the opposite.

    The following discussion was a bit difficult to follow as @devsnek edited his response about ten times, but in one of the first revisions he agreed with me.

  3. devsnek commented on Apr 15, 2018

    @devsnek
    Member

    I suggest you change it, as the way I see it isn't apparently very popular. I'll approve whatever pr is opened 👍

  4. jdalton commented on Apr 16, 2018

    @jdalton
    Member

    \cc @isaacs as author of the semver package for insight into API design.

  5. rvagg commented on Apr 16, 2018

    @rvagg
    Member

    Why is this even in core? This is the definition of feature creep, users should just be using the semver package to do this themselves rather than duplicating all that logic and prerelease rules.

    The whole of #19587 is a smell, we're duplicating existing functionality (the numbers themselves! what's wrong with a split() to get the major??) and functionality available via npm that includes some non-trivial rules that we need to make ensure is in sync (having dabbled in semver parsing and those rules I can say this is not straightforward).

    I'd +1 removing it entirely as a "fix", would I be alone in that?

  6. rvagg commented on Apr 16, 2018

    @rvagg
    Member

    proposing reversion @ #20062 along with some reasons and some notes on how the API is currently broken

  7. BridgeAR commented on Apr 22, 2018

    @BridgeAR
    MemberAuthor

    The whole API got reverted. Closing therefore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    metaIssues and PRs related to the general management of the project.processIssues and PRs related to the process subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions