Hybrid node:http + fetch approach to node-fetch-server #33
Conversation
…tch-hybrid-server
…n benchmark runner
… and hybrid FetchIncomingMessage constructpor for node:http
| /** | ||
| * **DO NOT USE**, will always throw. Kept only for compatibility with type definitions. | ||
| * | ||
| * @deprecated Use `Request.formDataAsync()` instead which returns an `AsyncGenerator<MultipartPart>` |
There was a problem hiding this comment.
Where is Request.formDataAsync defined? Is that a new spec? Or are you using the form-data-parser package?
| // const ac = new AbortController(); | ||
| // this.on('close', () => { | ||
| // ac.abort(); | ||
| // }); | ||
|
|
||
| // let signal = ac.signal; | ||
| // this.#signal = ac.signal; | ||
|
|
||
| // if (signal.aborted) { | ||
| // ac.abort(signal.reason); | ||
| // } else { | ||
| // // Keep a strong ref to ac while request object | ||
| // // is alive. This is needed to prevent AbortController | ||
| // // from being prematurely garbage collected. | ||
| // // See, https://github.com/nodejs/undici/issues/1926. | ||
| // this[kAbortController] = ac; | ||
| // } |
There was a problem hiding this comment.
Is this supposed to be uncommented? Seems like it, otherwise request.signal will be undefined.
| return http.createServer<IncomingMessage>( | ||
| { IncomingMessage }, |
There was a problem hiding this comment.
Interesting! I had no idea the createServer API could use a custom IncomingMessage class. Very cool find 👍
| }, | ||
| "scripts": { | ||
| "bench": "bash ./bench/runner.sh", | ||
| "build": "tsup", | ||
| "test": "node --import @swc-node/register/esm-register --test ./src/**/*.spec.ts", | ||
| "debug": "node --import @swc-node/register/esm-register ./src/test.ts", |
There was a problem hiding this comment.
It looks like ./src/test.ts is missing?
|
|
||
| // hybrid magic | ||
| export { FetchIncomingMessage } from './lib/fetch-incoming-message/index.js'; | ||
| export { createFetchServer } from './lib/create-fetch-server.js'; |
There was a problem hiding this comment.
Note: Ideally instead of creating two APIs we'd stick with a single API, createRequestListenener. Although I understand that you're exporting createFetchServer here under a different name so you can benchmark them both together.
| get host() { | ||
| return this.get('host'); | ||
| } |
There was a problem hiding this comment.
Can we please put this in a separate PR, and add a setter as well? Thanks 🙏
NOTE: all changes in this PR are non-breaking and implemented as separate, parallel exports except for:
added getter for the Host header to
@mjackson/headersupdated
platformfield inpackages/node-fetch-server/tsup.config.tsto'nodeimplemented a custom
IncomingMessageclass that anode:httpserver can use as-is, and that implements theRequestinterface fromundici-types5-10% faster than the base implementation in
node-fetch-server15-20% faster than
Expressadded some basic unit tests to check parsing of sample requests works