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

Extract input extensions in extractExtensionDefinitions #948

Merged
merged 4 commits into from
Sep 7, 2018
Merged

Extract input extensions in extractExtensionDefinitions #948

merged 4 commits into from
Sep 7, 2018

Conversation

jure
Copy link
Contributor

@jure jure commented Sep 5, 2018

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • 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
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

This is a sort of exploratory thing, so apologies if it doesn't tick all of the boxes. The story is that graphql added input extensions in graphql/graphql-js#1373 (14.0.0) and I expected this to automatically work with graphql-tools. But it's actually missing a thing from extractExtensionDefinitions that would make it 'just work'. So I added that in this PR, but since it also requires updating to graphql 14.0.0, some tests break.

Not sure how to proceed, so I'd be super happy for some guidance, and happy to brings this through to the end.

@apollo-cla
Copy link

@jure: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ghost ghost added the feature New addition or enhancement to existing solutions label Sep 5, 2018
@jure jure mentioned this pull request Sep 5, 2018
@ghost ghost added the feature New addition or enhancement to existing solutions label Sep 6, 2018
@jure
Copy link
Contributor Author

jure commented Sep 6, 2018

I made this MR more minimal, so that it doesn't require graphql 14.0.0, while still adding the functionality to extract the input extensions. I think this might now tick the boxes it needs ticking. Let me know how it looks!

Update: I think consensus for this feature was reached in #641

@hwillson hwillson self-assigned this Sep 7, 2018
@hwillson hwillson added the to do label Sep 7, 2018
Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

This is awesome @jure - thanks very much!

@hwillson hwillson merged commit 4bdc1fb into ardatan:master Sep 7, 2018
@hwillson hwillson removed the to do label Sep 7, 2018
@jure
Copy link
Contributor Author

jure commented Sep 7, 2018

Thanks @hwillson! 🎉

@jure
Copy link
Contributor Author

jure commented Sep 14, 2018

At the risk of sounding naggy, any chance of that prophesied 4.0.0 release? :)

@hwillson
Copy link
Contributor

@jure Haha - not naggy at all! I've been meaning to post back here with an update; I ran into a world of hurt trying to get graphql-tools internal enum capabilities working with graphql 14.0. There's light at the end of the tunnel though, and I'm prepping that PR now. Once that's in place (today), then I'll plan on cutting a new release hopefully right after. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants