express-graphql
express-graphql copied to clipboard
graphql
[Proposal] Split into multiple middlewares
Just a idea, but I think we should embrace express middleware system.
Maybe something like below:
import { parser, validate, execute, errorHandler, graphiql } from 'express-graphql';
import logger from 'graphql-third-party-query-param-logger';
const app = express();
app.use('/graphql', parser(...));
app.use('/graphql', logger(...));
app.use('/graphql', validate(...));
app.use('/graphql', execute(...));
app.use('/graphql', errorHander(...));
app.use('/graphql', graphiql(...));
app.listen(3000);
And then we can use middlewares at any points to log and measure performance, even have more control on input, output and errors.
Eventually, more reasonable, extendable middlewares will born and enrich the whole graphql ecosystem.
Relative issue: #101 #102 #107
I think something like this could be useful. What are the important use cases here?
For performance logging it does seem useful to be able to insert some middleware after the query is parsed, but before it is executed. You might also want to do things like rejecting a query that was too complicated at the same point.
It does seem like it is useful to mount graphiql separately but that doesn't seem like it should work like app.use('/graphql', graphiql(...)); - it seems more like you would want app.use('/graphql/graphiql', graphiql(...)); but then I don't really see how separating out the middleware buys you anything over just mounting graphiql on a different URL.
What is the benefit of separating validate, execute, and errorHandler?
This looks like a good and robust approach. IMO Logging & GraphiQL should be separated modules.
For validate, execute& errorHandler I can't see a practical use case since they are part of GraphQL core.
Sorry for the late reply, but I think this is an interesting idea as long as it's optional to not break the existing API.
For those who would like to approach this, consider unfolding one middleware out at a time for PRs rather than one huge PR that attempts to refactor everything at once ;)
FYI, just closed a couple of other related issues in the interests of centralizing the discussion here. For context, this is the comment I made on #178:
I think there's clearly a demand for this (as evidenced by this issue, and #113 and #101). In essence, all of these are about the same thing: making express-graphql more extensible and reusable. The actual mechanism we use to do that needs to be decided (whether it be splitting it out into smaller libraries that can be recomposed, or creating a structure that makes it easy for other libraries to insert themselves in some way), but I think there's value in the core, abstract idea independently of any implementation details.
We should centralize the discussion in a single place though, so I am going to close this issue and request that we concentrate further discussion in issue #113.
Let's carry on from here.
I think the most conservative option for this refactoring is to take advantage of Node's own API for HTTP stuff
middleware(req: http.ClientRequest, res: http.ServerResponse)
: Promise<http.ServerResponse>
I could work on a draft in the next days.
I like @chentsulin proposal.
Just add my 5cents:
For backward compatibility, we also should provide all-in-one middleware, like graphqlHTTP. And rewrite it with using granular middlewares. So newcomers can start with simple middleware and over time can migrate to granular implementation.
export function parser(opts) {
return (req, res, next) {
// ...
next();
}
}
export function validate(opts) {
return (req, res, next) {
// ...
next();
}
}
// ... other middlewares ...
// and the default middleware with callback hell 😈
export default function graphqlHTTP(opts) {
const parserMW = parser(opts_for_parser);
const validateMW = validate(opts_for_validate);
return (req, res, next) => {
parserMW(req, res, () => {
validateMW(req, res, () => {
executeMW(req, res, () => {
errorHanderMW(req, res, () => {
next(); // if needed
});
});
});
});
}
};
@nodkz seems reasonable. But I think it should be done in the new Express adapter. This package should just provide a toolset for such adapters.
If it provides some insight, in the end for graphql-api-koa it worked well having just 2 middlewares; one for errors (errorHandler) and one for executing (execute):
import Koa from 'koa'
import bodyParser from 'koa-bodyparser'
import { errorHandler, execute } from 'graphql-api-koa'
import schema from './schema'
const app = new Koa()
.use(errorHandler())
.use(bodyParser())
.use(execute({ schema }))
from the issue i just closed as a dupe of this:
yes, I think there's one design change we need to make. currently we are setup for connect-based http servers, so express works as does hapi, restify, etc if you see the docs. However, Koa is not connect based. There is koa-connect which is not reccomended. So we need a core server module that express-graphql exports, as well as a koaMiddleware however we will probably keep it to one package as @IvanGoncharov and i have discussed, but if folks feel seperate packages are important for particular reasons we can consider that.
So I've set up a GH project with this as an end goal, showing at least an express and koa reference middleware, and as already, some docs that explain how to use with other connect based http middleware stacks
I would definitely love something like this to help handle permissions.
Currently, I do the following:
- add authentication middleware
- handle permissions for each field, query, or mutation individually in each resolver by calling functions that handle the permissions from the resolvers.
I have to do permissions directly in the resolvers since we have some pretty complicated permissions going on, and it's a lot easier since I have the arguments object available for each field or query.
If I could put some middleware between the resolver and what parses the query string into a javascript object it would cut down on a lot of code re-use
