Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to semver ~1.2.3 style ranges #534

Merged
merged 2 commits into from
Jun 3, 2019
Merged

Conversation

focusaurus
Copy link
Contributor

@focusaurus focusaurus commented May 31, 2019

Impact: minor
Type: chore

Issue (new proposing ~1.2.3)

We mostly had caret-style ^1.2.3 semver ranges for our deps. This range permits minor version upgrades. This is in theory supposed to be OK and be backward compatible, but in practice we somewhat frequently see minor updates breaking our code. Most recently, some of our apollo deps started breaking the tests when we updated to resolve an unrelated synk security vulnerability.

I'm proposing a switch to ~1.2.3 ranges so we only get patch releases automatically/implicitly.

  1. Only permit patch upgrades
  • we do want patch upgrades so our install base can get security fixes right away without any interaction with us as upstream

Issue (old v1 proposing 1.2.x)

We mostly had caret-style ^1.2.3 semver ranges for our deps. This range permits minor version upgrades. This is in theory supposed to be OK and be backward compatible, but in practice we somewhat frequently see minor updates breaking our code. Most recently, some of our apollo deps started breaking the tests when we updated to resolve an unrelated synk security vulnerability.

I'm proposing a switch to 1.2.x x placeholder ranges for 2 reasons:

  1. Only permit patch upgrades
  • we do want patch upgrades so our install base can get security fixes right away without any interaction with us as upstream
  • This is also achievable with the ~1.2.3 syntax though
  1. I personally find the 1.2.x syntax much more clear and obvious. I have to repeatedly lookup the meaning of caret vs tilde

caveat we lose the ability to specify a minimum patch version like ~1.2.3 gave us. I don't believe that is currently an issue for us, but it could become in the future. If so, just use tilde for the dependency at hand.

Reference

npm semver calculator

Solution

Use ~1.2.3 syntax in all cases except for prerelease (alpha, beta, rc, etc) in which case use boolean style 2-part syntax (>=1.0.0-rc.1 < 1.1.0).

Breaking changes

None I'm explicitly aware of, although this does feel somewhat risky but I can't back up that gut feeling with concrete info.

Testing

  • Run docker-compose up on this branch with no preparation. It should just work and launch the app.
  • Make sure unit tests still pass (CI will handle this)

@rosshadden
Copy link
Contributor

I am not sure how I feel about this change. I've never seen this format used, and did not know it existed. The convention is definitely to use ~ which as you say lets us specify a minor patch version. I think losing that ability could be harmful. I agree ^ vs ~ were confusing when I first saw them but they are so ingrained in the Node culture right now that I think it's okay if people have to look it up should they not remember.

Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal @focusaurus!

I'm in favor of the move from floating minor versions (^) to floating patch versions. I agree with your assessment the minor version - while it's supposed to contain only backward compatible changes - too frequently does cause breakage. Perhaps this discussion should be focused on larger bits of our Reaction Platform (storefront, reaction/API, etc). We may need to have a separate discussion about this topic for JS libs/npm packages that we release.

I'd like to hear what others think about the move from the ~2.4.2 style to the 2.4.x style. Similar to Pete I have had to look up what the caret means vs the tilde more times than I'd like. At the same time, that style carries more information with it - such as the minimum version number - and can be used for a broader set of versions, including betas (-beta.1) and release candidates (-rc.1). Is this worth giving up in order to have a more readable syntax? I wasn't aware that the "0.64.x" syntax was valid until reading this proposal, so while it was more readable to me once I knew it was valid syntax, I was initially a little confused.

@rosshadden
Copy link
Contributor

I also don't think this format would play nicely with Synk, but that is a shooting-from-the-hip gut feeling.

@focusaurus
Copy link
Contributor Author

@spencern Yes I agree that the best semver strategy varies by project type (application, library, framework, etc) so I'm suggesting this specifically for storefront due to its specific circumstances as partially an application and partially a library/toolkit.

Also @spencern from what I can tell from https://semver.npmjs.com/, pre-release beta/rcs cannot use tilde, those need boolean style range comparisions.

@rosshadden I did a test npx snyk test and it worked but haven't tried snyk wizard.

The 1.2.x syntax is of very marginal/dubious benefit, and now that we've done this PR I don't think I'll have to look it up anymore, so I'm going to amend this to use ~1.2.3 ranges.

- Don't take unexpected minor releases
- Next commit will bump versions to match yarn.lock

Signed-off-by: Peter Lyons <[email protected]>
@spencern
Copy link
Contributor

spencern commented Jun 3, 2019

@focusaurus you're right that ~1.0.0 won't include ~1.0.0-rc.1. I played around with that tool and it appears that you can use something like ~1.0.0-rc.1 which will match any -rc.x as well as any patch version that comes after it such as 1.0.3 - I'm not sure if this would be desired behavior, so using boolean style ranges is probably the safest approach for this.

- Add fresh yarn.lock file

Signed-off-by: Peter Lyons <[email protected]>
@focusaurus focusaurus changed the title Switch to semver 1.2.x style ranges Switch to semver ~1.2.3 style ranges Jun 3, 2019
@focusaurus focusaurus merged commit 1efc1d9 into develop Jun 3, 2019
@focusaurus focusaurus deleted the chore-pete-semver-patch-x branch June 3, 2019 18:19
@aldeed
Copy link
Contributor

aldeed commented Jun 7, 2019

We had started pinning new deps to exact versions last year for essentially the same reasons. I’m fine with patch-only as the solution instead, but maybe you should also change all the pinned ones to be patch only?

@focusaurus
Copy link
Contributor Author

OK I changed the exact pins to tildes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants