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.
There was a problem hiding this comment.
I'm not sure this would work, wouldn't that make the WHATWG adapter reliant on node types?
I also see that the lambda adapter does not have an addTo implementation.
For now I changed the makeExampleServer function to receive a type parameter to revert the node behavior to addTo while keeping the different behavior for whatwg. Let me know if that works.
| @@ -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?
There was a problem hiding this comment.
Yeah I didn't like it either but couldn't find a way to get the HTTP version from a WHATWG Request object. Changed it to 1.1 but still hardcoded unfortunately
| 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?
There was a problem hiding this comment.
Tried changing this based on the other adapters, let me know if this looks better.
| }; | ||
| }, | ||
| 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.
There was a problem hiding this comment.
I'm honestly not sure about this. How can I tell if this can be set to false?
| 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.
Good catch, returning buffer as is.
| ), | ||
| ); | ||
| }) | ||
| } |
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
There was a problem hiding this comment.
See this comment #2288 (comment)
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).