Added whatwg-node-server adapter for grafserv #2288
Conversation
|
There was a problem hiding this comment.
Thanks for starting work on this; it would be great to add @whatwg-node/server as a supported server framework!
Please be sure to format your code; you can run yarn lint:fix in the root. May be worth installing a prettier extension into your editor of choice and configuring it so that it happens automatically on save.
We will also need some tests before this can be merged. Probably makes sense for makeExampleServer() in this file:
to accept an argument choosing the server framework to use - then we can test it on each different adaptor easily using the same test suite.
| @@ -4,7 +4,7 @@ import type { AddressInfo } from "node:net"; | |||
| import { constant, error, makeGrafastSchema } from "grafast"; | |||
| import { resolvePreset } from "graphile-config"; | |||
|
|
|||
| import { grafserv } from "../src/servers/node/index.js"; | |||
| import { grafserv } from "../src/servers/whtatwg-node-server"; | |||
| @@ -36,8 +36,7 @@ export async function makeExampleServer( | |||
| }); | |||
|
|
|||
| const serv = grafserv({ schema, preset }); | |||
| const server = createServer(); | |||
| serv.addTo(server); | |||
| const server = createServer(serv.createHandler()); | |||
There was a problem hiding this comment.
Please don't make this change, it does not handle websockets and other concerns. It means that serv is not passed an instance of server and thus cannot add any event listeners.
| @@ -115,6 +115,7 @@ | |||
| "@types/koa": "^2.13.8", | |||
| "@types/koa-bodyparser": "^4.3.10", | |||
| "@whatwg-node/fetch": "^0.9.10", | |||
| "@whatwg-node/server": "^0.9.64", | |||
There was a problem hiding this comment.
Looking at the code, this should also be an optional peer dependency, otherwise createServerAdaptor cannot be imported? (The other adaptors do not require anything but types be imported.)
| httpVersionMajor: 1, | ||
| httpVersionMinor: 0, |
There was a problem hiding this comment.
1.0 seems unlikely, that's ancient! Are we not able to determine the HTTP version from a WHATWG server?
| return { | ||
| httpVersionMajor: 1, | ||
| httpVersionMinor: 0, | ||
| isSecure: url.protocol === 'https://', |
There was a problem hiding this comment.
Are you sure this is correct? The protocol is actually https:, the // is not part of the protocol itself (see for example location.protocol in a browser).
https://developer.mozilla.org/en-US/docs/Web/API/URL/protocol
| const text = await request.text() | ||
| return { | ||
| type: "text", | ||
| text, | ||
| }; |
There was a problem hiding this comment.
Are we certain the body will always be text?
| }; | ||
| }, | ||
| requestContext: { | ||
| whatwg: {request} |
There was a problem hiding this comment.
This should include a version number: whatwgv1 or similar.
| requestContext: { | ||
| whatwg: {request} | ||
| }, | ||
| preferJSON: true, |
There was a problem hiding this comment.
Disappointing, this opts out of some performance enhancements.
| case "buffer": { | ||
| const { statusCode, headers, buffer } = response; | ||
| const respHeaders = new Headers(headers) | ||
| return new Response(buffer.toString('utf8'), {status: statusCode, headers:respHeaders}) |
There was a problem hiding this comment.
There's no guarantee that the buffer can safely be represented as a utf8 string; is it not possible to return binary data directly to the response?
| ), | ||
| ); | ||
| }) | ||
| } |
There was a problem hiding this comment.
Please also implement an addTo method to follow convention; e.g.:
crystal/grafast/grafserv/src/servers/node/index.ts
Lines 290 to 298 in 36c41e8
Description
@whatwg-node/server is an adapter that helps create generic servers across runtimes (https://www.npmjs.com/package/@whatwg-node/server). It could, at least in theory, replace all of the servers currently implemented, while supporting several more. If this all works out it should help integrations while having to maintain less code.
Note: This doesn't support web-sockets, I'm still looking for a way to do that in a generic fashion, but I see some of the other adapters don't support web-sockets either so I believe this is good enough for now.
Performance impact
unknown, but this PR won't affect anything by itself, it's just adding the adapter.
Security impact
None
Checklist
yarn lint:fixpasses.yarn testpasses (Well, sort of - I ran the grafserv tests with it and it passed)RELEASE_NOTES.mdfile (if one exists).