feat: HTTP server overhaul, improved framework support #1361
Conversation
|
This works as intended, however I'm currently planning to try and fix our Koa and Fastify support, and I think I can do it as part of this by rewriting these newly introduced |
|
Breaking out the handlers like this looks like an excellent change! It'll make some of our fronting proxy configurations simpler. |
|
I believe this is ready for merge now. Before doing so I'd love some confirmation it works nicely; @PaulMcMillan are you able to test this on your Restify stack? Assuming you have a pretty vanilla setup you should be able to:
Let me know if you need more help. I'd also appreciate any of our existing Koa/Fastify users test it in their setups. I don't intend this to be a breaking change, but it is pretty invasive! CC @madflow @jwickens @cdaringe @MaxDesiatov |
|
In particular, I'm interested to know how it interacts with more complex setups with lots of middlewares (compress, etc) |
|
I'm not currently using postgraphile right now, but split handlers are a welcome change. 👍 |
|
@benjie I'll work on getting this running in our stack, should be able to give you feedback by mid next week. |
|
@PaulMcMillan excellent 🙌 |
|
I guess I am in This does not work and my ideas what is expected end here ... I would assert that I more likely qualify as a Fastify user than a Fastify handler developer ;) |
|
So it turns out we only had fastify v2 support; and v3 removes the middleware support we were relying on, so I've done fastify v3 support in the same way that restify support will work (using an adaptor much like the one @madflow wrote above). A fastify v3 PostGraphile server will look something like: const fastify = require('fastify');
const fastifyFormBodyParser = require('fastify-formbody');
const { postgraphile, PostGraphileResponseFastify3 } = require('postgraphile');
const handler = postgraphile(...);
const app = fastify();
// Natively, Fastify only supports 'application/json' and 'text/plain' content types
// Add application/graphql:
app.addContentTypeParser(
'application/graphql',
{ parseAs: 'string' },
(request, payload, done) => done(null, payload),
);
// And application/x-www-data-urlencoded:
app.register(fastifyFormBodyParser);
// Takes a PostGraphile route handler and turns it into a fastify route handler
const convertHandler = handler => (request, reply) =>
handler(new PostGraphileResponseFastify3(request, reply));
// Wrapping in a function allows you to register these routes under a subpath should you wish
const routes = (router, opts, done) => {
router.options(handler.graphqlRoute, convertHandler(handler.graphqlRouteHandler));
router.post(handler.graphqlRoute, convertHandler(handler.graphqlRouteHandler));
if (handler.graphiqlRouteHandler) {
router.head(handler.graphiqlRoute, convertHandler(handler.graphiqlRouteHandler));
router.get(handler.graphiqlRoute, convertHandler(handler.graphiqlRouteHandler));
}
if (handler.faviconRouteHandler) {
router.get('/favicon.ico', convertHandler(handler.faviconRouteHandler));
}
if (handler.eventStreamRouteHandler) {
router.options(handler.eventStreamRoute, convertHandler(handler.eventStreamRouteHandler));
router.get(handler.eventStreamRoute, convertHandler(handler.eventStreamRouteHandler));
}
done();
};
app.register(routes, { prefix: "/api" });
app.listen(3000);(I wrote this in the GitHub comment box, so let me know if I made mistakes.) |
Working!
should be added, if this was intended. I tested with a few Fastify plugins (etag, cors, helmet) - all work. Very nice! Subpath via 👍 🥇 |
|
Missing line added; thanks! That GraphiQL isn't working means that the |
|
(And thanks so much for taking the time to test this!) |
|
I got this working within our Restify framework, and the parts I tested all seemed to work well. I didn't test the eventStreamRoute functionality since our ecosystem makes websockets tricky. |
|
Thanks @PaulMcMillan! (Technically the |
|
Given we've had issues in the past with compression of requests, particularly relating to event streams, I correctly guessed that we would again. To track them down I've built servers for each of the main frameworks we now support:
For all but node I've made two versions: I had to dig into the code of the various servers to try and figure out how to achieve this. I'd love if people more familiar with these frameworks would like to step forward and let me know how I can achieve this in PostGraphile core without users having to worry about it. This |
|
Before I can merge this PR, I need to go back through all these examples and see if they work for subscriptions. 😅 |
|
Amazingly subscriptions worked across all 9 servers (vanilla + rum&raisin) without any changes! 🙌 |
benjie
changed the title
|
These changes are released in |
| const externalUrlBase = options.externalUrlBase || ''; | ||
| const graphqlRoute = options.graphqlRoute || '/graphql'; | ||
| const graphqlRoute = | ||
| subscriptionServerOptions?.graphqlRoute || |
There was a problem hiding this comment.
This optional chaining syntax does not work for me in the Docker build. I get:
/postgraphile/build-turbo/postgraphile/http/subscriptions.js:40
const graphqlRoute = subscriptionServerOptions?.graphqlRoute ||
^
SyntaxError: Unexpected token '.'
There was a problem hiding this comment.
Thanks for raising this! Fixed in #1376
In future, please file an issue rather than commenting on a closed PR; it makes it easier to track 🙏
Description
Adds support for
restify, and overhauls the HTTP server in general, fixing a number of longstanding issues in the process.Separate HTTP handlers for /graphql, /graphiql, /graphql/stream and /favicon.ico
Fixes #1354
Fixes #1220
We've broken all the handlers out so you can call them directly. This allows you to opt into the pieces of functionality that you care about, mounting just those things to the relevant routes. E.g. Restify support looks like this:
The
handlerToMiddlewareconverter takes one of the PostGraphile route handlers as an argument and returns a middleware suitable for Restify, Express, Connect or Node.js's built in HTTP. For Koa and some other servers you'd need a different adaptor; adaptors inherit fromPostGraphileResponseand there are two built in adaptors:PostGraphileResponseNodeandPostGraphileResponseKoa. The PostGraphile route handlers are:middleware.graphqlRouteHandler- serves GraphQL requests (i.e. those typically to/graphql)middleware.graphiqlRouteHandler- serves the GraphiQL interface (i.e. the HTML typically served from/graphiql)middleware.faviconRouteHandler- serves the PostGraphile faviconmiddleware.eventStreamRouteHandler- serves the PostGraphile watch-mode event stream pointed to atX-GraphQL-Event-Stream; typically/graphql/streamURL-rewriting proxy support
Fixes #1232
Previously we confounded two separate concerns with the
externalUrlBaseoption, which meant that people who have proxy servers that rewrite the URLs could not get GraphiQL to work reliably. We now add theexternalGraphQLRouteoption that explicitly states where a browser would find the GraphQL endpoint relative to the GraphiQL interface, so that GraphiQL can always talk to GraphQL (both over HTTP and websockets). Separately, we've added internal logic that derives the sub-path the middleware was mounted on and passes this detail through to the websocket server such that it should Just Work ™️ (e.g. if you haveapp.use("/subpath", postgraphile({graphqlRoute: "/gql"}))then PostGraphile will know to listen for websocket connections on/subpath/gql). If you're usingenhanceHttpServerWithSubscriptionsdirectly and you mount your middleware on a sub-route within express or similar framework, then you should figure out this path for yourself and pass it as thegraphqlRouteparameter, e.g.enhanceHttpServerWithSubscriptions(server, middleware, { graphqlRoute: "/subpath/gql" }).If the above is a bit confusing, you can see a working proxy + subpath setup in
scripts/proxied-test-server.js.Websocket
Performance impact
Negligible.
Security impact
No known issues.