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

Allow to pass function that returns Promise as a rootValue option #253

Closed
wants to merge 1 commit into from

Conversation

IvanGoncharov
Copy link
Member

This functionality is required when you need different rootValue depending on the query. In our case, we are working on a GraphQL proxy that needs to pass a query to the original GraphQL server and provide its response as the rootValue.

@IvanGoncharov
Copy link
Member Author

@wincent @leebyron Can you please review this PR?
We already released our tool and was forced to use our own fork instead of official NPM package:
https://github.com/APIs-guru/graphql-faker/blob/master/package.json#L41

@leebyron
Copy link
Contributor

This is actually already supported! However instead of allowing each property in this configuration to be an async function, providing the configuration as a whole is provided as an async function.

You can chain promises (or await async functions) in whatever order makes the most sense for your configuration before returning it to be used.

For example:

app.use('/graphql', graphqlHTTP(async (request) => ({
  schema: MyGraphQLSchema,
  rootValue: await myPromisedRootValue(request)
})));

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Mar 28, 2017

@leebyron My bad, I didn't state it clearly in PR description, but we already tried this approach.

Problem is that request parameter comes directly from Express.
That means if I need arguments I have to parse them similar to parseGraphQLParams. But you can't call it twice on a same Request object because body is Stream and can't be read twice.
In order to apply my transformations, I also have to parse query document into AST.
Moreover, If I call visit and visitWithTypeInfo combo with an invalid query (e.g. a query that uses unknown type) it will crash which means I also have to duplicate validation.
So at this point, I reimplemented half of express-graphql functionality just to find out that I also need to track if GraphiQL should be displayed to not proxy query in this case.

This PR adds support for the callback that is called with query AST and only if query passed validation. I believe this functionality may be required for other complicated tools.

@leebyron
Copy link
Contributor

Sorry for letting this sit. Thanks for the additional context.

It sounds like there is a separate issue at hand then. This is a fixing the symptom of the problem, but not the problem itself, which is that you want the order of parsing the request and running the async function to return options to happen in the reverse order, which I think is a good idea.

leebyron added a commit that referenced this pull request May 15, 2017
This ensures that the `request` parameter to the `options()` function is already endowed with metadata about the GraphQL request so that it may be used when constructing options.

Addresses #253
@leebyron
Copy link
Contributor

I've added #302 which ensures the ordering of creating params and options will then ensure request has the properties you need.

Please let me know if that addresses the concern

leebyron added a commit that referenced this pull request May 15, 2017
This ensures that the `request` parameter to the `options()` function is already endowed with metadata about the GraphQL request so that it may be used when constructing options.

Adds a third argument to the function for easier access to graphql specific data.

Addresses #253
leebyron added a commit that referenced this pull request May 15, 2017
This ensures that the `request` parameter to the `options()` function is already endowed with metadata about the GraphQL request so that it may be used when constructing options.

Adds a third argument to the function for easier access to graphql specific data.

Addresses #253
@IvanGoncharov
Copy link
Member Author

@leebyron #302 Solves only the first part of the problem of getting raw parameters. After that, I still have to parse the query to AST and validate since I can’t use visit and visitWithTypeInfo combo with an invalid query (e.g. a query that uses unknown type) because it will crash.

Moreover, at the time when options CB is called, I don’t know if the request should be processed by the server or just return HTML for GraphiQL. And I think that it introduces many complications beyond our use-case. For example, if you use this CB for logging, you will also log queries passed to GraphiQL and log it the second time when GraphiQL sends the same request to the server which is very confusing and not intuitive.

So in the end, I had to copy-paste ~200 lines into my code.

@IvanGoncharov
Copy link
Member Author

Closing in favor of #391.

IvanGoncharov added a commit that referenced this pull request Apr 7, 2019
This PR allows providing your own `execute` function instead of the default one from the `graphql-js`.
It’s a small change that adds a lot of flexibility since you can wrap standard `execute` and it opens new possibilities for middlewares. 

Personally, I need it to proxy calls to 3rd-party APIs.
It’s an alternative to #253 but introduces less code and more flexible mechanism.
It’s very important for us as it will be the last change required to switch [graphql-faker](https://github.com/APIs-guru/graphql-faker) from custom forks of `graphql-js` and `graphql-express`.
@wincent @leebyron Can you please review it?
junminstorage pushed a commit to junminstorage/express-graphql that referenced this pull request Aug 14, 2020
This PR allows providing your own `execute` function instead of the default one from the `graphql-js`.
It’s a small change that adds a lot of flexibility since you can wrap standard `execute` and it opens new possibilities for middlewares. 

Personally, I need it to proxy calls to 3rd-party APIs.
It’s an alternative to graphql#253 but introduces less code and more flexible mechanism.
It’s very important for us as it will be the last change required to switch [graphql-faker](https://github.com/APIs-guru/graphql-faker) from custom forks of `graphql-js` and `graphql-express`.
@wincent @leebyron Can you please review it?
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants