Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Allow custom graphql params extraction from request #730

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TrySound
Copy link
Contributor

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.

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
Copy link
Contributor Author

cc @IvanGoncharov

@acao
Copy link
Member

acao commented Dec 12, 2020

excellent idea for decoupling from express.

after a quick review:

  • should there be a change to package lock without dependency changes?
  • potentially the function could be named more descriptively? also i think “custom” is a given in this case

@acao
Copy link
Member

acao commented Dec 12, 2020

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

@TrySound
Copy link
Contributor Author

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?

src/index.ts Outdated
* An optional function which will get params from request instead of
* default getGraphQLParams
*/
customGraphQLParamsFn?: (
Copy link
Member

@acao acao Jan 3, 2021

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?

Copy link
Contributor Author

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.

@TrySound
Copy link
Contributor Author

TrySound commented Jan 6, 2021

@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
Copy link
Contributor Author

Hi @acao. What do you think?

@acao
Copy link
Member

acao commented Jan 16, 2021

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

src/index.ts Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jan 17, 2021

@TrySound You can parse body yourself and replace body inside Request

// If express has already parsed a body as a string, and the content-type
// was application/graphql, parse the string body.
if (typeof body === 'string' && typeInfo.type === 'application/graphql') {
return { query: body };
}

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 #675

@TrySound
Copy link
Contributor Author

TrySound commented Jan 17, 2021

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.

Base automatically changed from master to main February 10, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants