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

chore: update yarn.lock to resolve snyk js-yaml vuln #531

Merged
merged 1 commit into from
May 31, 2019

Conversation

focusaurus
Copy link
Contributor

@focusaurus focusaurus commented May 31, 2019

  • Change apollo-related package.json semver ranges to tilda. Otherwise we get stuff too new and that causes unit test failures with Invariant Violation: Ambiguous GraphQL document: contains 2 operations
  • update yarn.lock to fix js-yaml vulnerability
  • remove ignored vulnerabilities as they should now be resolved

Also changed everything apollo related to semver ~ ranges.
Otherwise new versions cause test failures.

(Invariant Violation: Ambiguous GraphQL document: contains 2 operations)

Signed-off-by: Peter Lyons <[email protected]>
@focusaurus focusaurus force-pushed the chore-update-deps-for-snyk-js-yaml branch from 377d27b to 48d1668 Compare May 31, 2019 17:21
@focusaurus
Copy link
Contributor Author

OK yay this finally passed circleci.

@focusaurus focusaurus requested a review from nnnnat May 31, 2019 17:29
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.

What's the thought behind switching from floating minor version (^) to floating patch (~) version? Should we update the rest of the package.json file to follow this pattern as well? Is this decision specifically targeted at apollo related packages?

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.

Didn't read through the description well enough where the tilde vs caret is explained (thanks Pete). Looks good to me.

@spencern spencern merged commit 969d6a3 into develop May 31, 2019
@spencern spencern deleted the chore-update-deps-for-snyk-js-yaml branch May 31, 2019 18:07
@focusaurus
Copy link
Contributor Author

@spencern If we leave the carets for apollo stuff, an update breaks the unit tests with Invariant Violation: Ambiguous GraphQL document: contains 2 operations. The switch to tilde is a smaller range and prevents us jumping minor versions at least until we have time to actually deal with the new graphql query invariant check. I tried to change as little as possible. If I were to re-think our semver approach, I'd suggest we always use 7.6.x syntax because

  1. We want to allow patch fixes mostly for security updates
  2. We want to avoid minor updates because they often are breaking even though they are not supposed to be
  3. The .x syntax for me personally is way clearer. I have to always double check the docs to understand the tilde and caret rules. However, in rare cases the .x might need to be replaced when we require >= a specific patch version.

@spencern
Copy link
Contributor

@focusaurus
I agree with your decision to change as little as possible in this particular PR. I'd be interested in seeing a DR/Proposal for rethinking our semver approach. My experience matches yours that minor versions too frequently do contain breaking changes.
I don't have strong attachment to the existing way of specifying versions (~ 7.6.1) but I'd want to hear others opinions on that before making that change.

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.

3 participants