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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try using a field extension for 1.9 interpreter #99

Merged
merged 7 commits into from Feb 13, 2019

Conversation

rmosolgo
Copy link
Contributor

@rmosolgo rmosolgo commented Feb 11, 2019

This doesn't quite work yet, and I think it's because of the changes in rmosolgo/graphql-ruby#1984. (Previously, there were cases when execution would proceed eagerly down the tree, but in that PR, it was changed to always resolve each level of depth before proceeding to further depths.)

I think this means that the interpreter has to hardcode handling for mutation root fields, which isn't too surprising. I'll take a look. Fixed by rmosolgo/graphql-ruby#2097

This adds a CI build for 1.9.0.pre4 and and implementation that uses a FieldExtension. I think it will work pretty well, the only problem will be if people do:

# DONT DO THIS, IT WOULD BE BAD 馃懟
use GraphQL::Batch
mutation(Types::Mutation)

It will still work for now because plugins are applied after other configurations. But in the future, there will be no "build" step, and instead, the schema will be constructed just as configured in the source, so it might start breaking.

We could override self.mutation(t) to raise an error, to avoid that configuration error. Would you like to?

(Also, I realize you may prefer a different path, depending on the conversation over at #93, but I thought I'd code one up just to see how it played out.)

@swalkinshaw
Copy link
Contributor

I'm in favour of is even if it ends up being a shorter term solution 馃憤It would be good to have a version that supports 1.9 before we consider a larger re-org or unification?

@rmosolgo
Copy link
Contributor Author

rmosolgo commented Feb 12, 2019

Same here, I think it'd be great to release a graphql-batch version that will support 1.9.0 so that people can update OK. (I've been doing the same for graphql-client.) Anything else I can do on this one, or does it look OK?

lib/graphql/batch.rb Outdated Show resolved Hide resolved
dylanahsmith and others added 2 commits February 13, 2019 07:09
Co-Authored-By: rmosolgo <rmosolgo@github.com>
@rmosolgo
Copy link
Contributor Author

馃憤 Thanks for taking a look! I applied that suggestion and cleaned up the now-needless require at the end of the file.

@dylanahsmith dylanahsmith merged commit 07505a3 into Shopify:master Feb 13, 2019
@swalkinshaw swalkinshaw temporarily deployed to rubygems February 13, 2019 17:57 Inactive
@rmosolgo rmosolgo deleted the 1.9-interpreter branch February 13, 2019 19:19
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.

None yet

3 participants