-
-
Notifications
You must be signed in to change notification settings - Fork 469
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
feat: support batch graphql request handling #1585
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 60b6920:
|
@kettanaito any thoughts on this? Also, sorry to tag you directly, I'm just not sure who else to maybe tag on this lol. There's nobody else that's even close to as active as you are, so it's not clear who is actually active in contributing... |
Hi, @johnboulder. First things first, thanks for proposing and implementing this! What concerns getting this merged and released, the library is on hold for new features such as this one because I don't have the capacity to migrate those features to v2.0 that's in its final stages of development at the moment. This means that I will get to this pull request and we will, very likely, merge it once v2.0 is out (i.e. #1404). Expect some time this summer if everything goes smoothly. |
Hey! Good to hear from you! That's awesome news! My team is currently using a forked release that includes my changes, and we'd love to get off it as soon as possible. So I'll be happy to make any updates to this (if they're needed) as soon as v2.0 is released. If v2.0 comes out and I don't follow up with resolving any merge conflicts on this feel free to tag me. I may just not have noticed the release. |
Released: v2.1.3 🎉This has been released in v2.1.3! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Closes #510
This might be more of a bandaid than a final solution, but I figure something is better than nothing. For the team I'm working with, fixing this is kindof critical to the work we're doing (which I hope just clarifies why I'm approaching it this way). It seemed like the best way to fix the problem without making a larger set of changes.
I've really only been looking at this for the past few days so I'm just doubting whether this contribution is thorough enough. Everything I've tested with this works fine though, so it feels like a nice bandaid.
Approach:
The core of the change is in
handleRequest()
. What I've done is made it so it just checks if the incoming request is a batched graphql request. If it is, we iterate over the body creating individual requests and callinggetResponse()
with them. Once that finishes executing, the code just wraps that up in an array, serializes it, and returns it as the body of the response. I get the impression that this might be circumventing the logic that comes after callinghandleRequest()
, but I struggled to find documentation around that in particular, and it seems like it might be hard to treat a batched request exactly the same as a normal graphql request.I feel like this just a nice, temporary solution for the time being while folks decide on something more thorough.