Skip to content

In-house batching #378

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

Closed
wants to merge 2 commits into from
Closed

In-house batching #378

wants to merge 2 commits into from

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Nov 5, 2016

For reasons (#274), the gem should have built-in support for batched resolution: one pass to gather needs, then resolve those needs together, then dole out the results to the fields that asked for them. This is what graphql-batch does so well, but moving it to core library code will make things a bit easier to maintain.

Todo

  • Add query-level batching
  • Implement null propagation
  • Support nested loaders
  • Check graphql-batch tests for any cases I've missed

@cjoudrey
Copy link
Contributor

cjoudrey commented Nov 5, 2016

cc @dylanahsmith

@dylanahsmith
Copy link
Contributor

Why not focus supporting asynchronous resolvers, since batching can be implemented on top of it in addition to concurrent and parallel execution?

For instance, the overrides in graphql-batch are purely to support async field resolution, so bringing that in-house would remove the coupling between the two libraries, which still allowing for experimentation with different ways of doing the execution.

Right now this PR seems to couple batch loading to resolving a field in a way that only allows a single batch load per field. Since a resolver may need to do multiple queries that should be batched and will need to do something with that result, I suspect you will end up with something more similar to promises/futures to provide the more general solution, so I don't think you should start with the current coupling of a field to a single batch loader.

I'll try to take a stab at providing a minimal solution for async support.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 7, 2016

Yes, I'd love to see that! It's hard for me to understand quite what the right extension point is, so any help in that direction would be very welcome!

@junosuarez
Copy link

I agree that async field resolution is the right extension point here. It should be possible without changing the API or adding an async: true flag or anything like that, depending on the promise or future implementation.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Nov 8, 2016

@dylanahsmith I took another crack at at last night. If you can take a look, I'd love to hear your feedback: #386

In any case, it's much better than this one!

@rmosolgo rmosolgo closed this Nov 8, 2016
@rmosolgo rmosolgo deleted the batching branch November 8, 2016 21:35
@rmosolgo rmosolgo restored the batching branch November 11, 2016 07:37
@rmosolgo rmosolgo deleted the batching branch November 17, 2016 01:50
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.

4 participants