feat(fastify): Integrate apollo-fastify plugin #626 [WIP] #1760
Conversation
|
|
||
| if (this.playgroundOptions && request.method === 'get') { | ||
| // perform more expensive content-type check only if necessary | ||
| const accept = parseAll(request.headers); |
There was a problem hiding this comment.
I would use https://github.com/fastify/fastify-accepts instead.
There was a problem hiding this comment.
I don't think this is working right now, the usage is different.
There was a problem hiding this comment.
@mcollina I need a next() method along with async/await but based on the docs
Notice: the next callback is not available when using async/await or returning a Promise. If you do invoke a next callback in this situation unexpected behavior may occur, e.g. duplicate invocation of handlers.
we can't use next() along with async/await. What would be an alternative in this case?
| 'Content-Length', | ||
| Buffer.byteLength(JSON.stringify(graphqlResponse), 'utf8'), | ||
| ) | ||
| .send(JSON.parse(graphqlResponse)); |
There was a problem hiding this comment.
This is really not clear. Why are you serializing the response back-and-forth again? You are probably better off as not parsing/stringifying both the body and the request/response. This likely halves the throughput of this solution.
There was a problem hiding this comment.
Removing the serialization breaks most of the tests. I am assuming that the response need to be serialized.
There was a problem hiding this comment.
I mean that you are getting JSON via runHttpQuery, which I imagine it is parsed and stringified again, or maybe I'm mistaken by the API.
addityasingh
force-pushed
the
as-integrate-fastify-plugin
branch
from
1ff69c7
to
d78d51a
Compare
| }); | ||
|
|
||
| if (next && typeof next === 'function') { | ||
| next(); |
There was a problem hiding this comment.
A question to clarify? Shouldn't this return back to the previous context if the callback is invoked, if next() is available.
E.g. I am assuming at this point that the invocation of the callback is what matters, which else wise could render undefined at some point?
if (next && typeof next === 'function') {
next();
return;
}
|
@addityasingh any updates here? I would like to help to speed things up. |
|
@addityasingh As I mentioned in #2280, I didn't realize we had two PRs in flight that built the same integration. The names were so similar, that I just realized after I merged that PR. Furthermore, you're the actual current maintainer of the You've put some effort into this integration, in both in v1 and v2, and I think you deserve commit credit, despite the fact that #1971 ended up landing. I merged this PR into #2280, preferring that branches implementation, but you'll see your commit credit. I'll close this now, with it being superseded by #1971 and @2280, but thanks for doing this originally. I'm working on catching up on the back-log to help reduce the chance of this in the future. We're still looking for your blessing on the |
|
@abernix Yeah. I opened 2 PRs (#1013 , #1760 ) to get the |
Refer #626 , #1013
TODO: