Skip to content

Suggestion: ban fancy semver ranges #43

Closed

Description

@hdgarrood

I have felt for quite a long time that semver ranges like ^x.y.z or ~x.y.z are an antipattern, because they obscure what the actual bounds are, and they encourage you to tighten lower bounds prematurely. For instance, lots of JS tools push you towards using a caret range with whatever version you happened to be using when you were writing the code. Of course, it's a reasonable default at first if you're adding a dependency at version x.y.z to declare the bounds as >=x.y.z <(x+1).0.0 (note: this is what ^x.y.z means as long as x >= 1), but if you later make a change to support newer versions of that dependency, you often won't need to tighten the lower bound at the same time. That is, if I've written ^x.y.z and I want to allow my package to be used with a new version (x+1).0.0 of the same dependency, caret ranges encourage me to just write ^(x+1).0.0, which means that my package can no longer be used with any x.* version. Instead, if I was forced to write >=x.y.z <(x+1).0.0 to begin with, it's easy to see how to relax the upper bound without tightening the lower bound.

To be explicit, my proposal is that the only acceptable version range operators are >, >=, <, <=, and ==.

Note that the caret or tilde ranges aren't part of the semver spec; as far as I'm aware, they were first introduced by the node semver package. Elm does this already (and in fact goes a little further), by requiring that all version ranges are of the form a.b.c <= v < d.e.f.

Activity

  1. f-f commented on Jul 11, 2020

    @f-f
    Member

    Great point. I would totally go as far as Elm does. I have only one question about this: how should we import packages from Bower? Should we convert the caret range to the explicit range?

  2. hdgarrood commented on Jul 11, 2020

    @hdgarrood
    ContributorAuthor

    I think so, yes. I guess the alternative is allowing existing packages to use carets and at some point disallowing new uploads from using them? I don’t have a strong preference but I feel like since we are generating the manifest files for existing versions in an automated manner anyway, we might as well convert them during the initial import?

  3. f-f commented on Jul 11, 2020

    @f-f
    Member

    Yes, I agree that in this case we have the option of converting everything, so we should just go for that.
    However there is always the questions of: do we remove packages/versions if we cannot convert them? (in this initial import)

    E.g. while trying to mass-convert all Bower packages the first time I found out that ~20 packages were just unreachable (I assume because the owners just took the repos down). In this case taking the decision to just remove them from the index is straightforward, but there are trickier cases, e.g. when it comes to the name-constraints (I didn't check yet if any package would fail the constraints we defined in #3)

    Intuitively I'd be fine just removing non-compliant stuff that we cannot convert in any way, but I guess it's good to keep somewhere a list of "things that were removed due to $reason"

  4. hdgarrood commented on Jul 11, 2020

    @hdgarrood
    ContributorAuthor

    I may be wrong, but I think it should always be possible to rewrite any version range which bower understands in a format which would be acceptable according to this proposal.

  5. thomashoneyman commented on Jul 11, 2020

    @thomashoneyman
    Member

    What about attempting to adjust a package to the simple bounds, and dropping it / opening a PR to update it if that can’t be done? I’m happy to take on opening PRs to packages that have to be updated so their version bounds work.

  6. f-f commented on Jul 11, 2020

    @f-f
    Member

    I may be wrong, but I think it should always be possible to rewrite any version range which bower understands in a format which would be acceptable according to this proposal.

    @hdgarrood I agree with that, in my previous comment I am asking what we should do in the general case.

  7. f-f commented on Jul 11, 2020

    @f-f
    Member

    @thomashoneyman the problem in this case is that published versions are considered immutable, so there's no "update" operation that can be performed, we have to say either "yes" or "no".

    For future packages/versions we can just have CI reject packages if the ranges don't fit our constraints, so there shouldn't be the need for us to do updates to repos

  8. hdgarrood commented on Jul 12, 2020

    @hdgarrood
    ContributorAuthor

    Ah right, I see. I don't mind losing a few, but if we find we are rejecting lots of packages which are still in use, then we should probably reconsider. Perhaps if need be we could apply more relaxed rules to package versions which existed before the initial import?

  9. f-f commented on Mar 8, 2021

    @f-f
    Member

    Update: I'm looking at this and trying to implement something using the semver package.

    It looks like:

    • it's not possible to automatically convert a SemVer caret range to a simple range like the one specified here as "maximum version" of a caret range doesn't seem to be a defined concept, see [QUESTION] Why doesn't semver.maxVersion exist? npm/node-semver#337
    • we cannot get a "parse tree" or even answer the question "what kind of range is this?", so we'd have to have a custom parser to detect "simple ranges" or something like that

    At this point I am not sure anymore if it's worth adding this constraint at all vs just issuing a recommendation client-side in the publishing flow. This is because in the latter case we don't have to be accurate since it's just a recommendation, while in the CI here we need to be 100% correct so that everything can be automated

  10. hdgarrood commented on Mar 9, 2021

    @hdgarrood
    ContributorAuthor

    Issuing a recommendation client-side sounds fine to me! However I don't think it's true that it's impossible to convert a caret range to a simple >=a.b.c <d.e.f range. The node-semver package readme does give some examples, even:

    https://github.com/npm/node-semver#caret-ranges-123-025-004

    • ^1.2.3 := >=1.2.3 <2.0.0-0
    • ^0.2.3 := >=0.2.3 <0.3.0-0
    • ^0.0.3 := >=0.0.3 <0.0.4-0

    I suspect the question being asked in the issue you linked, which is impossible to answer, is "given a caret range, what's the maximum version that is within the range?" This is not defined because caret ranges always have a strict upper bound such as <2.0.0, i.e. an upper bound that uses < rather than <=. If you have a strict upper bound, say <2.0.0, you can always add one to the patch version, i.e. 1.x.y -> 1.x.(y+1), so there's no maximum version which satisfies that bound. That's fine though - we don't need to answer the question of "what is the maximum version within the range," because we can just sidestep the issue by providing a strict upper bound.

  11. f-f commented on Apr 7, 2021

    @f-f
    Member

    In #140 we found out that indeed it's possible to convert to "simple ranges" without implementing it ourselves, e.g. see an example conversion from the tests:
    https://github.com/purescript/registry/blob/3d091b10d63350e8d374612e0eabbf825394cb40/ci/test/Main.purs#L140

    Currently we convert the ranges when importing from Bower, I'll leave this issue open to track the fact that we should error out if the ranges in the Manifest are not simple.

  12. thomashoneyman commented on Oct 18, 2021

    @thomashoneyman
    Member

    There is a widespread convention of depending on any version of a package, up to the next major version (or up to the next minor version).

    I worry about these simple ranges being problematic in that folks who would usually write ^1.0.0 or ^1.3.4 will start to write >1.0.0 <2.0.0, which can opt them in to pre-releases (ie. that range will include 2.0.0-rc.1 -- Halogen's release candidates come to mind -- but not the actual 2.0.0 release).

    They really need to write >1.0.0 <2.0.0-0, but this isn't obvious and I'm going to predict that most people won't do it. I suggest that we take one of these courses of action:

    1. We support < <= == >= >, as @hdgarrood suggested, and we also support ^ because of how widely it's used
    2. We only support simple ranges, but we implicitly treat the end version as the actual end version (ie. < 2.0.0 is considered to actually mean < 2.0.0-0).
    3. We don't allow you to publish versions with build metadata / release candidate information to the registry. If you want to use one of those packages you'll have to point to it directly, not via the registry.

    Elm inspired the original discussion here, and I'm fairly sure they don't even allow things like build or pre-release information to be in their version ranges, which means they can do < 2.0.0 with no worries -- it really will restrict to the 1.x.x version series.

  13. f-f commented on Oct 20, 2021

    @f-f
    Member

    I think option (3) is not viable in the first place, because we are importing many packages that have pre-release versions.
    I don't have an opinion yet on what's the best course of action here, but I wonder: do we have to do anything in the first place about this, i.e. is it such a big issue?

    There are two scenarios where this can happen:

    1. a package with overly lax bounds is published
    2. an application has overly lax bounds on a package

    For case (1) we could simply restrict the bounds (either with authors publishing a new version, or with Trustees fixing the bounds for old packages), and for case (2) the application developers can just fix the bounds locally as soon as they notice the breakage.

  14. thomashoneyman commented on Oct 20, 2021

    @thomashoneyman
    Member

    I think option (3) is not viable in the first place, because we are importing many packages that have pre-release versions.

    For option 3 to be viable we would drop packages with prerelease versions. Perhaps that's not acceptable! (You could still depend on those packages by pointing at their Git url, those specific versions just wouldn't be in the registry.)

    I wonder: do we have to do anything in the first place about this, i.e. is it such a big issue?

    Something feels wrong to me that the obvious bounds to write (ie. >=2.0.0 <3.0.0) are incorrect, and so the design of these version bounds will lead people to write incorrect bounds. Overly lax bounds can be fixed, but ideally we'd avoid the issue by not making it so easy to write subtly incorrect bounds in the first place.

    ...but perhaps in practice this is rare enough that we don't worry about it. I don't have good data on this.

  15. thomashoneyman commented on Oct 21, 2021

    @thomashoneyman
    Member
  16. f-f commented on Dec 1, 2021

    @f-f
    Member

    Is this something that we'll need to resolve before shipping the first version?

  17. f-f commented on Dec 1, 2021

    @f-f
    Member

    TODO:

    • run a community poll about this in Discourse
    • add the check for banning fancy ranges in teh API pipeline
  18. thomashoneyman commented on Jan 6, 2022

    @thomashoneyman
    Member

    Community poll:

    https://discourse.purescript.org/t/rfc-disallow-build-metadata-and-prerelease-identifiers-in-the-registry

    We also need to spec what ranges are allowed as input, which I realize we haven’t done. For example, what does <1.0.0 ==1.2.3 mean? Is it legal?

  19. f-f commented on Jan 6, 2022

    @f-f
    Member

    Yeah, I meant to add a check to the API pipeline to only allow only simple ranges when adding new packages (but not when importing legacy ones) - this is the reason why we had Range originally defined as { original :: String, converted :: String }, setup that was removed in #229 because we forgot what it was for 😂

    This check should just verify that original == converted (since the converted ranges are always simple)

  20. f-f commented on Jan 9, 2022

    @f-f
    Member

    Uh, I just realized that if we introduce the check mentioned in the comment above - i.e. verifying that all the ranges in a manifest are the same as the ranges that parseRange returns us, something like >=1.3.4 <2.0.0-0 - then there's no point in removing RC releases, since they don't fit in such range (as you noted above):

    > semver.satisfies("2.0.0", ">=2.0.0 <3.0.0-0");
    true
    > semver.satisfies("2.9.99", ">=2.0.0 <3.0.0-0");
    true
    > semver.satisfies("3.0.0", ">=2.0.0 <3.0.0-0");
    false
    > semver.satisfies("3.0.0-0", ">=2.0.0 <3.0.0-0");
    false
    > semver.satisfies("3.0.0-rc1", ">=2.0.0 <3.0.0-0");
    false

    Indeed in all of this I am assuming that the only range we'd accept is in the form of >=A.B.C <X.Y.Z-0.
    I'd say this is fine because:

    • we'd check this client side when publishing a package, and clearly communicate to users that their ranges need to be in that form
    • introducing a different acceptance criteria would require us to reimplement parseRange from scratch, instead of reusing the js implementation, which is something that I'm not really keen on
  21. thomashoneyman commented on Jan 19, 2022

    @thomashoneyman
    Member

    This is my proposed specification for accepted versions and ranges to start with. I can open a new issue if that would be better. If it generally looks good, then I'd like to open a PR enforcing this check in the registry and the legacy import tool.


    Registry Versions & Version Ranges

    The PureScript registry allows packages to specify a version for themselves and version ranges for their dependencies.

    We use a restricted version of the SemVer spec which only allows versions with major, minor, and patch places (no build metadata or prerelease identifiers) and version ranges with the >= and < operators.

    This decision keeps versions and version ranges easy to read, understand, and maintain over time.

    Package Versions

    Package versions always take the form X.Y.Z, representing major, minor, and patch places. All three places must be natural numbers. For example, in a manifest file:

    {
      "name": "my-package",
      "version": "1.0.1"
    }

    If a package uses all three places (ie. it begins with a non-zero number, such as 1.0.0), then:

    • MAJOR means values have been changed or removed, and represents a breaking change to the package.
    • MINOR means values have been added, but existing values are unchanged.
    • PATCH means the API is unchanged and there is no risk of breaking code.

    If a package only uses two places (ie. it begins with a zero, such as 0.1.0), then:

    • MAJOR is unused because it is zero
    • MINOR means values have been changed or removed and represents a breaking change to the package
    • PATCH means values may have been added, but existing values are unchanged

    If a package uses only one place (ie. it begins with two zeros, such as 0.0.1), then all changes are potentially breaking changes.

    Version Ranges

    Version ranges are always of the form >=X.Y.Z <X.Y.Z, where both versions must be valid and the first version must be less than the second version.

    When comparing versions, the major place takes precedence, then the minor place, and then the patch place. For example:

    • 1.0.0 is greater than 0.12.0
    • 0.1.0 is greater than 0.0.12
    • 0.0.1 is greater than 0.0.0

    All dependencies must take this form. For example, in a manifest file:

    {
      "name": "my-package",
      "version": "1.0.1",
      "targets": {
        "lib": {
          "my-dependency": ">=1.0.0 <2.0.0",
          "my-dependency-2": ">=2.1.5 <2.1.6",
          "my-dependency-3": ">=0.1.0 <12.19.124"
        },
      }
    }
  22. JordanMartinez commented on Jan 19, 2022

    @JordanMartinez
    Contributor

    Just a thought, once a library hits 1.0.0, "release candidate" releases could be implemented on top of this if people followed a convention. If all "odd" major numbers represent actual releases, then "even" major numbers could indicate a release candidate for the upcoming release.

    • 1.0.0 - actual release
    • 2.0.0 - release candidate for 3.0.0 (i.e. the real v2)
    • 3.0.0 - actual release (i.e. the real v2)

    Not sure if this is worthwhile to enforce because it would get confusing pretty quickly.

  23. f-f commented on Jan 19, 2022

    @f-f
    Member

    @JordanMartinez while such a scheme might be appealing to some folks, I would personally not like this to be enforced on my version numbers.
    SemVer is designed in such a way that makes it easiest to spot breaking changes, and I feel that's what we should keep as our first priority. The scheme above blurs this, as it would force a breaking release even if there are no actual breaking changes.

  24. JordanMartinez commented on Jan 19, 2022

    @JordanMartinez
    Contributor

    @f-f Yeah, I don't disagree with you. While the above idea can be implemented, I think it's too confusing.I also agree that banning these 'fancy' ranges will remove a lot of ambiguity. Hence why it was "just a thought." 😁

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions