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

Pin graphql to the ~14.2.x range #1291

Merged
merged 1 commit into from
May 22, 2019
Merged

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented May 22, 2019

An incoming graphql release (14.3.1) is going to break at least
apollo client:check as it did in the past with 14.2.0 (confirmed this
manually with graphql's master branch). This dependency restriction
will prevent this for now, without us needing to find the root of
the issue. This is preventative until we have the resources
to investigate and resolve the actual cause of the problem.

For more details and full context, see:
graphql/graphql-js@183ff32#r32971387

cc @abernix @martijnwalraven

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

An incoming graphql release (14.3.1) is going to break at least
`apollo client:check` as it did in the past (confirmed this
manually with graphql's master branch). This dependency restriction
will prevent this for now, without us needing to find the root of
the issue. This is preventative for now until we have the resources
to investigate and resolve the actual cause of the problem.

For more details and full context, see:
graphql/graphql-js@183ff32#r32971387
@trevor-scheer trevor-scheer requested a review from abernix May 22, 2019 21:58
@trevor-scheer trevor-scheer merged commit 36d718e into master May 22, 2019
@trevor-scheer trevor-scheer deleted the trevor/pin-graphql-14.2.x branch May 22, 2019 22:03
@abernix
Copy link
Member

abernix commented May 22, 2019

I'm slightly worried that this will cause duplicated graphql versions (which is a problem because of graphql) for those that have already upgraded to 14.3.0, but we should be defensive for the time-being. At least in the case that I've just described, there would be very clear failures pretty quickly in the application, rather than less-likely-to-be-noticed surprise differences due to unexpected changes in graphql.

@ryami333
Copy link

ryami333 commented Jun 4, 2019

@trevor-scheer is there anything I can do to help move this along? Is this a result of a breaking change inadvertantly introduced by the graphql-js maintainers, and if so: are they aware of it? I could not find any mention of this in their recent issues so I don't suppose there's any upstream fix coming, which implies that the responsibility to fix lies here instead.

@trevor-scheer
Copy link
Member Author

@ryami333, you're welcome to investigate and I'm happy to share what information I have with you. There's some history with trouble with recent patches of graphql on our end. It's a lot to explain, but most of the conversation happened here:
graphql/graphql-js@183ff32#r32971387

14.2.0 breaks the CLI, but the resulting fix which works for the CLI (14.2.1) was a breaking change in and of itself. This was pointed out after 14.3.0, which resulted in the 14.3.1 patch.

Prior to the release of 14.3.1, I set the version to ~14.2.1 which currently works for the CLI.

If you're interested in proposing a fix and have the time to investigate, you can reproduce by changing the graphql version to latest and running apollo client:check
See this comment: graphql/graphql-js@183ff32#r33630399

I haven't had time to investigate the issue, but I'd be happy to help anyone who's interested in doing so. Unfortunately, this path was chosen as a way to minimize damage (^14.2.1 would resolve to 14.3.1 and break the CLI), but you're correct in noting that the problem isn't solved.

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