Skip to content

http surface for subclassed IncomingMessage and ServerResponse - #4

Open
puskuruk wants to merge 11 commits into
mainfrom
fix/http-surface-for-subclassed-request-response
Open

puskuruk wants to merge 11 commits into
mainfrom
fix/http-surface-for-subclassed-request-response

Conversation

@puskuruk

@puskuruk puskuruk commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

What @nestjs/platform-node needs from node:http, node:net and URL: ServerOptions.IncomingMessage/ServerResponse constructors, ServerResponse<Req> with req, URL.canParse defined through the constructor, absent headers reading as undefined, and socket readable/writable flags that on-finished reads, on net.Socket and on the HTTP server's own req.socket.

  • a764850 Make http.ServerResponse generic over its IncomingMessage and expose req
  • 94f7625 Honour ServerOptions.IncomingMessage and ServerOptions.ServerResponse constructors in createServer
  • b753a7f Add URL.canParse to the compiled WHATWG URL class
  • 4860248 Cover ServerResponse subclassing through ServerOptions and URL.canParse with node-vs-compiled parity tests
  • 5fc7133 Type IncomingMessage headers as string | undefined so a header the request did not send reads as undefined, with a dictionary-missing parity test
  • 7e9691b Maintain net.Socket's readable and writable flags across connect, end, peer end and destroy, as the on-finished shim reads them
  • 54dd13d Build hono-bridge's Request headers from the header values the request carries, now that an absent IncomingMessage header reads as undefined
  • 230e902 Report an accepted HTTP connection's socket as open, and closed once destroyed
  • bdcd9e1 Throw ERR_INVALID_URL from the URL constructor and define canParse through it
  • 390ebf5 Drop the deviations the typed request surface removed from the README
  • 579b980 Assert the dictionary probe matches Node line for line, on compiler 1.0.47 or newer

Verification on 579b980 (compiler 1.0.47): HTTP_SERVER_OPTIONS_PARITY_OK:5, URL_CAN_PARSE_PARITY_OK:170, DICTIONARY_MISSING_PARITY_OK:22; full sweep green (hono-hello build, test-hono, fastify, 31 scripts).

test-dictionary-missing.mjs compares the compiled probe with Node line for line. The two lines it used to record as a compiler gap (a missing key of a { [k: string]: string } map under typeof and ??) match since compiler 1.0.47 (geastack/compiler#15), so the compiler requirement moves from ^1.0.43 to ^1.0.47 (peer, dev, lockfile): the floor the parity holds from.

Related

Rebased on main 3d53505 (after #12 and the 1.0.21 release). #2 and #3 are merged. Branch nest/base is the base of #6 (Nest 1/4).

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f5034bc9-f77d-435e-8bdc-8e1b10d403c7
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown

Add Carrot credits or activate Agent usage billing to use Autofix

@puskuruk

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@puskuruk

Copy link
Copy Markdown
Collaborator Author

Rebased on main 6bb33da (after #11). Review points, each checked on the current head 7bbff81:

1. http.Socket readable/writable: fixed in 5d61c70. An accepted connection's socket now sets readable/writable to true (runtime/node/http.ts:316). destroy() sets both back to false (:353). req.socket.writable is true for a live response, so on-finished no longer treats it as finished.

2. Dictionary test passing over a miscompile: fixed in 7bbff81. The skip set is gone. The two lines are a compiler gap: a missing key of { [k: string]: string } reads as "" under typeof and ??, where Node gives undefined. The test now:

  • asserts each gap's exact compiled line and prints it as KNOWN GAP
  • reports DICTIONARY_MISSING_MATCHED:20 KNOWN_GAPS:2 instead of PARITY_OK
  • fails with "now matches Node; remove it from knownGaps" once a compiler gets it right

The gap is still there on 1.0.43 and 1.0.46. geastack/compiler#15 fixes both lines, and with it the test is 22/22.

3. URL.canParse vs the constructor: fixed in 0410f6c. The validation is in the constructor's parse, which throws Node's TypeError with code: 'ERR_INVALID_URL', input and base. canParse is try { new URL(url, base); return true } catch { return false }, so new URL('http://a<b') throws and canParse returns false. The probe now has a constructor-outcome table over the same inputs (URL_CAN_PARSE_PARITY_OK:170).

4. Cleanups: done in 5d61c70.

  • serverResponseClass_ keeps its undefined default because the compiler refuses to carry the generic ServerResponse class as a value (native-boundary:uncensused-class-object). A comment there says so.
  • The stale "placeholder socket (connId 0)" text is gone from loadPeer.

Verification on 7bbff81 with compiler 1.0.43: full sweep green, including HTTP_SERVER_OPTIONS_PARITY_OK:5, URL_CAN_PARSE_PARITY_OK:170, STREAM_LIFECYCLE_PARITY_OK:27 and ZLIB_STREAMS_PARITY_OK:65.

…quest did not send reads as undefined, with a dictionary-missing parity test
…, peer end and destroy, as the on-finished shim reads them
…t carries, now that an absent IncomingMessage header reads as undefined
…destroyed

net.Socket's constructor leaves a handle `opening`, neither readable nor
writable, and only its own connect path flips those; http.Socket called
`super()` and never did, so a live request's `req.socket.writable` read
false and on-finished treated every response as already finished. A Socket
built for an accepted connection now reports what net.Socket reports once
connected, and `destroy()` reverses it the way net.Socket's does.

`serverResponseClass_` keeps its undefined default and now says why: the
compiler refuses to carry the generic `ServerResponse` class as a value
(`native-boundary:uncensused-class-object`). The `loadPeer` comment no
longer refers to a module-load-time placeholder socket that does not exist.
…rough it

The constructor's minimal parse accepted any text, while `URL.canParse` was
a separate validator, so the two could disagree. The validation now runs in
the constructor, which throws Node's `TypeError: Invalid URL` with `code`,
`input` and `base`, and `canParse` is what Node defines it as: the
constructor does not throw. The probe prints a constructor-outcome table
over the same inputs as the canParse table.
`res.req` exists, and a `req.headers` value is a string or undefined rather
than a plain string; the set-cookie deviation stays.
….0.47 or newer

`test-dictionary-missing.mjs` recorded two compiler gaps as known: a
missing key of a `{ [k: string]: string }` map read as `""` under
`typeof` and `??`, where Node reads `undefined`. Compiler 1.0.47
(geastack/compiler#15) observes the absence at both, and with it the
compiled probe matches Node on every one of its 22 lines. The known-gap
map and its bookkeeping go; the test compares each line and names the
three probe lines it exists for.

The parity holds from 1.0.47, so `@geastack/compiler` moves from
`^1.0.43` to `^1.0.47` in both the peer and the dev dependency, and the
lockfile follows.
@puskuruk
puskuruk force-pushed the fix/http-surface-for-subclassed-request-response branch from 7bbff81 to 579b980 Compare October 24, 2026 20:28
@puskuruk

Copy link
Copy Markdown
Collaborator Author

Rebased on main 3d53505 (after #12 and the 1.0.21 release). Head is now 579b980.

Compiler 1.0.47 (geastack/compiler#15) closes both gaps the dictionary test recorded: with it the compiled probe matches Node on all 22 lines (DICTIONARY_MISSING_PARITY_OK:22), and the same test on 1.0.46 fails at line 3 as it should. The test now asserts line-for-line parity and the compiler requirement moves from ^1.0.43 to ^1.0.47 (peer, dev, lockfile), since that is the floor the parity holds from.

Full sweep on 579b980 with 1.0.47: hono-hello build, test-hono, both fastify tests and all 31 scripts exit 0 (DICTIONARY_MISSING_PARITY_OK:22, HTTP_SERVER_OPTIONS_PARITY_OK:5, URL_CAN_PARSE_PARITY_OK:170).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant