search

graphql / express-graphql

Create a GraphQL HTTP server with Express.
MIT License
6.34k stars 538 forks source link

Allow custom graphql params extraction from request #730

Open TrySound opened 3 years ago

TrySound commented 3 years ago

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.

TrySound commented 3 years ago

cc @IvanGoncharov

acao commented 3 years ago

excellent idea for decoupling from express.

after a quick review:

acao commented 3 years ago

also i think it makes sense to append this to the options interface rather than introducing a second argument

TrySound commented 3 years ago

Hi @acao

also i think it makes sense to append this to the options interface rather than introducing a second argument

It would look better in options though options getter depends on parsed options.

See here https://github.com/graphql/express-graphql/blob/master/src/index.ts#L231

should there be a change to package lock without dependency changes?

Probably not. I just committed what npm i gives. I usually use yarn which does not introduce any changes unless packages upgraded explicitly.

potentially the function could be named more descriptively? also i think “custom” is a given in this case

I don't know, maybe customGetGraphQLParams?

TrySound commented 3 years ago

@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.

TrySound commented 3 years ago

Hi @acao. What do you think?

acao commented 3 years ago

@TrySound you did move the option to where I was hoping, awesome!

IvanGoncharov commented 3 years ago

@TrySound You can parse body yourself and replace body inside Request https://github.com/graphql/express-graphql/blob/0fa7ace4f94fc25708a9f1e68a29775ceb623a25/src/parseBody.ts#L34-L38

Also, I'm worried that it will affect our effort to standardize the HTTP protocol for GraphQL: https://github.com/graphql/graphql-over-http As a reference GraphQL over HTTP server, we provide the ability to use non-standard names like graphqlQuery.

In my app I'm trying to move away from express.

We also have a plan to do that so we can support Koa, AWS Lambdas, Deno frameworks, etc. We currently blocked by TS migration of graphql-js but this is planned for the 1.0 release, see https://github.com/graphql/express-graphql/issues/675

TrySound commented 3 years ago

You can parse body yourself and replace body inside Request

Yes, I do request pollution now though it's not very clean.

Also, I'm worried that it will affect our effort to standardize the HTTP protocol for GraphQL:

My change allows to move everything to userland and use any non-standard names. Any framework could be integrated here as well.