fix(types): split http1 and http2 handler types #154
Conversation
📝 WalkthroughWalkthroughIntroduce protocol-specific Node HTTP types (HTTP/1 and HTTP/2), relax handler parameter typings by casting node request/response to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/adapters/_node/adapter.ts (1)
45-48: Consider adding JSDoc for the new type.The
NodeHttp1Handlertype definition correctly represents Express-style handlers. Consider adding a brief JSDoc comment to document its purpose for future maintainers.📝 Optional: Add documentation
+/** + * Node.js HTTP/1.x handler type for compatibility with libraries like Express. + */ type NodeHttp1Handler = ( req: NodeHttp.IncomingMessage, res: NodeHttp.ServerResponse, ) => void | Promise<void>;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/adapters/_node/adapter.ts
🔇 Additional comments (2)
src/adapters/_node/adapter.ts (2)
1-1: LGTM!The type-only import is appropriate for defining the
NodeHttp1Handlertype and adds no runtime overhead.
54-69: The type union addition is safe. WhileNodeHttp1Handleris cast toNodeHttpHandleron lines 62 and 66, both handler types are compatible at runtime becauseWebIncomingMessageextendsNodeHttp.IncomingMessageandWebServerResponseextendsNodeHttp.ServerResponse, which satisfy both signatures. The actual objects passed to the handler are always HTTP/1 types, making the cast valid.
pi0
changed the title
pi0
changed the title
pi0
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types.ts (1)
368-380: Middleware type split correctly mirrors handler pattern.The middleware types follow the same HTTP/1/HTTP/2 separation pattern as the handlers, which is consistent and correct. This allows Express-style middleware (HTTP/1) to be properly typed.
Note: There's a naming inconsistency between handler and middleware types:
- Handlers use
NodeHttp1Handler/NodeHttp2Handler(mixed case "Http")- Middleware use
NodeHTTP1Middleware/NodeHTTP2Middleware(uppercase "HTTP")If this inconsistency existed before this PR, it can be ignored. If introduced here, consider aligning to
NodeHttp1Middleware/NodeHttp2Middlewarefor consistency.🔎 Optional refactor for naming consistency
-export type NodeHTTP1Middleware = ( +export type NodeHttp1Middleware = ( req: NodeHttp.IncomingMessage, res: NodeHttp.ServerResponse, next: (error?: Error) => void, ) => unknown | Promise<unknown>; -export type NodeHTTP2Middleware = ( +export type NodeHttp2Middleware = ( req: NodeHttp2.Http2ServerRequest, res: NodeHttp2.Http2ServerResponse, next: (error?: Error) => void, ) => unknown | Promise<unknown>; -export type NodeHTTPMiddleware = NodeHTTP1Middleware | NodeHTTP2Middleware; +export type NodeHttpMiddleware = NodeHttp1Middleware | NodeHttp2Middleware;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/types.ts
🔇 Additional comments (1)
src/types.ts (1)
356-366: Type split is architecturally sound but Express compatibility claim is unsupported.While the separation of HTTP/1 and HTTP/2 handler types is correct and improves type clarity, the specific claim that Express apps "explicitly match
NodeHttp1Handler" is contradicted by the test code. The Express handler on line 78 oftest/node-adapters.test.tsrequires an explicitas NodeHttpHandlertype assertion, which indicates that Express types do not naturally match eitherNodeHttp1Handleror the union type without forcing it. Additionally, there is no reference to issue #149 in the codebase to confirm this change addresses that specific problem.The type split is a good improvement for code organization and type safety, but the review should avoid claiming it resolves Express compatibility issues without stronger evidence.
Likely an incorrect or invalid review comment.
pi0
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/adapters/_node/call.tssrc/adapters/_node/web/fetch.tstest/node-adapters.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/adapters/_node/call.ts (2)
examples/node-handler/server.ts (1)
handler(4-7)src/types.ts (1)
NodeHttpHandler(366-366)
test/node-adapters.test.ts (1)
src/types.ts (1)
NodeHttp1Handler(356-359)
src/adapters/_node/web/fetch.ts (1)
examples/node-handler/server.ts (1)
handler(4-7)
🔇 Additional comments (1)
test/node-adapters.test.ts (1)
43-96: Good: Expanded test coverage for Express and Fastify.Adding Express and Fastify fixtures provides valuable test coverage for the PR's objective of supporting HTTP/1 handlers from popular frameworks. The fixtures test different handler patterns (native Node.js, Express middleware, Fastify async handlers) which strengthens confidence in the adapter's compatibility.
However, note the type assertion concerns flagged separately - the tests should validate type compatibility without requiring
as anycasts.
| header: req.headers["x-test"] === "1", | ||
| body: req.body?.test === true, | ||
| }); | ||
| }) as NodeHttp1Handler, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the test file and view lines around 78, 93, 104
echo "=== Checking test/node-adapters.test.ts file size ==="
wc -l test/node-adapters.test.ts
echo -e "\n=== Lines 70-110 of test/node-adapters.test.ts ==="
sed -n '70,110p' test/node-adapters.test.ts
echo -e "\n=== Search for NodeHttp1Handler type definition ==="
rg 'type NodeHttp1Handler|interface NodeHttp1Handler' -A 5
echo -e "\n=== Search for NodeHttp1Handler export ==="
rg 'export.*NodeHttp1Handler' -B 2 -A 2Repository: h3js/srvx
Length of output: 2204
🏁 Script executed:
#!/bin/bash
echo "=== Find fetchNodeHandler definition and signature ==="
rg 'function fetchNodeHandler|const fetchNodeHandler|export.*fetchNodeHandler' -A 5
echo -e "\n=== Search for fetchCallers definition in test file ==="
sed -n '1,80p' test/node-adapters.test.ts
echo -e "\n=== Check the caller implementations ==="
rg 'class.*Caller|function.*Caller' -A 10 test/node-adapters.test.ts
echo -e "\n=== Look at what happens without the as any cast ==="
sed -n '95,115p' test/node-adapters.test.tsRepository: h3js/srvx
Length of output: 3923
🏁 Script executed:
#!/bin/bash
echo "=== Check toNodeHandler implementation ==="
rg 'export.*toNodeHandler|function toNodeHandler' -A 15
echo -e "\n=== Check if there's type acceptance for Express/Fastify apps ==="
rg 'Express.*Application|Fastify.*Instance' src/
echo -e "\n=== Look at the complete second caller definition more carefully ==="
sed -n '30,48p' test/node-adapters.test.ts
echo -e "\n=== Check PR or issue context in comments ==="
head -50 test/node-adapters.test.tsRepository: h3js/srvx
Length of output: 4065
🏁 Script executed:
#!/bin/bash
echo "=== Get full view of fetchNodeHandler function ==="
sed -n '1,30p' src/adapters/_node/web/fetch.ts
echo -e "\n=== Check what NodeHttpHandler includes ==="
rg 'type NodeHttpHandler|interface NodeHttpHandler' -B 2 -A 2
echo -e "\n=== Verify the actual test expectations - lines 100-120 ==="
sed -n '100,120p' test/node-adapters.test.ts
echo -e "\n=== Check if there are any utility functions to wrap Express/Fastify ==="
rg 'express|fastify' src/ -l
echo -e "\n=== Look for any type overloads for NodeHttpHandler ==="
rg 'NodeHttpHandler' src/types.ts -B 2 -A 5Repository: h3js/srvx
Length of output: 2918
🏁 Script executed:
#!/bin/bash
echo "=== Check the callNodeHandler implementation ==="
sed -n '1,50p' src/adapters/_node/call.ts
echo -e "\n=== Search for any type overloads or conditionals handling Express/Fastify ==="
rg 'typeof.*express|typeof.*fastify|instanceof.*Application|instanceof.*FastifyInstance' src/
echo -e "\n=== Look at complete adapter.ts for any special handling ==="
cat src/adapters/_node/adapter.ts
echo -e "\n=== Check if Express/Fastify have __nodeHandler properties ==="
rg '__nodeHandler' src/Repository: h3js/srvx
Length of output: 4143
Test bypasses type validation with casts that hide Express/Fastify incompatibility.
The test uses type assertions at three locations:
- Line 78:
as NodeHttp1Handlerfor Express app - Line 93:
as NodeHttp1Handlerfor Fastify routing - Line 104:
as anywhen callingfetchNodeHandler
These casts reveal that Express.Application and Fastify's routing don't naturally match NodeHttp1Handler (which expects a (req: IncomingMessage, res: ServerResponse) => void | Promise<void> function signature). The as any cast at line 104 bypasses type checking entirely, preventing validation that the code properly accepts these handlers without type errors.
To properly validate Express/Fastify support, remove the as any cast on line 104. If type errors appear, the underlying handler type definitions need adjustment rather than working around them with casts. The fixtures' type declarations and casts on lines 78 and 93 should also be eliminated if the PR's goal is true type compatibility.
🤖 Prompt for AI Agents
In test/node-adapters.test.ts around lines 78, 93 and 104, the test is
force-casting Express and Fastify handlers to NodeHttp1Handler and using `as
any` when calling fetchNodeHandler, which hides real type incompatibilities;
remove the `as NodeHttp1Handler` casts at lines 78 and 93 and the `as any` on
line 104, then fix the type definitions instead of masking errors — either
adjust the fixture types to implement the exact (req: IncomingMessage, res:
ServerResponse) => void | Promise<void> signature or broaden the
NodeHttp1Handler/fetchNodeHandler type declarations (or add overloads/generic)
so Express.Application and Fastify routing types are accepted by the handler
callsite; run type-checks and update tests until tsc passes without casts.
2 participants
resolves #149
This is a type-only compatibility improvement for the
toFetchHandlerutility, making it compatible with libraries that only support HTTP/1, e.g., express.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.