-
Notifications
You must be signed in to change notification settings - Fork 538
Allow custom graphql params extraction from request #730
base: main
Are you sure you want to change the base?
Conversation
In my app I'm trying to move away from express. I implemented own body parser to not pollute `request` object. Though in this case express-graphql is trying to parse body by itself which leads to hanging server because there are no more events on request. In this diff I added a way to custom extracting params from request. It would look better in options though options getter depends on parsed options.
excellent idea for decoupling from express. after a quick review:
|
also i think it makes sense to append this to the options interface rather than introducing a second argument |
Hi @acao
See here https://github.com/graphql/express-graphql/blob/master/src/index.ts#L231
Probably not. I just committed what
I don't know, maybe |
src/index.ts
Outdated
* An optional function which will get params from request instead of | ||
* default getGraphQLParams | ||
*/ | ||
customGraphQLParamsFn?: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TrySound is there a reason you didn't want to add this to the Options
interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot because of options getter which depends on params. If we could get rid of it Im good to move into options
It would look better in options though options getter depends on parsed
options.
@acao I figured how to move it into options. Now builtin getGraphQLParams is called only for options getter. When only object is passed first look for customGraphQLParamsFn field. |
Hi @acao. What do you think? |
@TrySound you did move the option to where I was hoping, awesome! |
@TrySound You can parse body yourself and replace body inside Request express-graphql/src/parseBody.ts Lines 34 to 38 in 0fa7ace
Also, I'm worried that it will affect our effort to standardize the HTTP protocol for GraphQL:
We also have a plan to do that so we can support Koa, AWS Lambdas, Deno frameworks, etc. |
Yes, I do request pollution now though it's not very clean.
My change allows to move everything to userland and use any non-standard names. Any framework could be integrated here as well. |
In my app I'm trying to move away from express. I implemented own
body parser to not pollute
request
object. Though in this caseexpress-graphql is trying to parse body by itself which leads to hanging
server because there are no more events on request.
In this diff I added a way to custom extracting params from request.
It would look better in options though options getter depends on parsed
options.