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

GraphQL dependency update (0.8.*) #4

Closed
wants to merge 2 commits into from

Conversation

mario-jerkovic
Copy link

@mario-jerkovic mario-jerkovic commented Nov 21, 2016

I want to resolve #3 by updating library code as the new GraphQL 0.8.* has a breaking change:

A resolve function's info.fieldASTs value has been renamed to info.fieldNodes (#554)

@tjmehta
Copy link
Owner

tjmehta commented Nov 21, 2016

Checking this out soon!
On Mon, Nov 21, 2016 at 9:42 AM Mario Jerkovic [email protected]
wrote:

Updated code as the new GraphQL 0.8.* has a breaking change:

A resolve function's info.fieldASTs value has been renamed to

info.fieldNodes (#554) graphql/graphql-js#554

You can view, comment on, or merge this pull request online at:

#4
Commit Summary

  • Updated info keys to reflect changes on updated graphQL dependencies
  • Code cleanup

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAnFF9ifmThr03vcSN_C8F9OdD6kHYVQks5rAdgOgaJpZM4K4fbN
.

@tjmehta
Copy link
Owner

tjmehta commented Nov 21, 2016

Wow this kind've sucks. I wish GraphQL used semver. I'll have to think about this a bit. I could release a breaking change myself and just put a note in the readme to use 2.x.x for graphql@^0.8.x.

Thoughts?

@mario-jerkovic
Copy link
Author

Maybe it's better to just have an additional condition that checks for presence of fieldASTs or fieldNodes keys. That way you will keep the compatibility with GraphQL 0.7.*

@tjmehta
Copy link
Owner

tjmehta commented Nov 22, 2016

@mario-jerkovic, great idea. Seems obvious in hindsight haha.. I think I jumped to a conclusion too quickly. Will take over this PR implement compatibility w/ both, fix the tests, and release soon :)

@tjmehta
Copy link
Owner

tjmehta commented Nov 22, 2016

continuing work in #5

@tjmehta tjmehta closed this Nov 22, 2016
@tjmehta
Copy link
Owner

tjmehta commented Nov 22, 2016

Added support in [email protected]

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.

GraphQL dependency update (0.8.*)
2 participants