🎉 feat: specialize response mapping via AOT for known schema types#1799
🎉 feat: specialize response mapping via AOT for known schema types#1799DenisCDev wants to merge 2 commits intoelysiajs:mainfrom
Conversation
WalkthroughAdded an optional adapter hook Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Request
participant Composer as Composer
participant Validator as Schema Validator
participant Adapter as Adapter
participant Runtime as Runtime Response
Client->>Composer: invoke composeHandler(route)
Composer->>Validator: getResponseSchemaKind(validator)
Validator-->>Composer: kind or null
alt specialization available
Composer->>Adapter: specializedResponse(kind, r, hasSet, saveResponse)
Adapter-->>Composer: specialized code (string) or undefined
Composer->>Composer: emit guard for kind
Composer->>Runtime: fast-path conditional -> specialized code
else fallback
Composer->>Runtime: emit generic mapResponse dispatch
end
Runtime-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapter/web-standard/index.ts`:
- Around line 22-28: In specializedResponse (the switch branch handling 'Object'
and 'Array') the hasSet=true path returns new Response(JSON.stringify(...),
c.set) without ensuring a content-type header; change that path to merge
c.set.headers with {'content-type':'application/json'} when not already present
(i.e., create a headers object combining c.set.headers and the content-type
fallback) so the Response always includes application/json; update the hasSet
branch that references saveResponse, r, and c.set to build a merged init before
calling new Response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8515709-e8cc-41d0-92b9-2270d3ba7e6b
📒 Files selected for processing (4)
src/adapter/bun/index.tssrc/adapter/types.tssrc/adapter/web-standard/index.tssrc/compose.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/adapter/web-standard/index.ts (1)
22-41: Verify BunAdapter parity for content-type behavior.The BunAdapter (lines 224-244 in bun/index.ts) relies on
Response.json()which auto-setscontent-type: application/jsonper the Fetch spec, and doesn't set explicit headers for String/Number/Boolean. This means the two adapters will produce differentcontent-typeheaders for the same response types:
Kind WebStandard (hasSet=true) Bun (hasSet=true) Object/Array application/json ✓ application/json (via Response.json) ✓ String text/plain ✓ (none from adapter) Number/Boolean (none) (none) If this divergence is intentional, consider documenting it. If not, consider aligning both adapters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapter/web-standard/index.ts` around lines 22 - 41, The WebStandard specializedResponse currently sets 'content-type:text/plain' for String responses when hasSet=true, which diverges from the BunAdapter that relies on Response.json() for objects/arrays and does not set text/plain for strings; to align behavior, update specializedResponse (function specializedResponse) to stop adding 'content-type:text/plain' for kind==='String' when hasSet is true (i.e., treat String the same as Number/Boolean: do not force a content-type), or alternatively add the explicit text/plain header in the Bun adapter where Response is created—pick one approach and make both adapters consistent (update the String branch in specializedResponse and mirror the change in the BunAdapter Response creation logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapter/web-standard/index.ts`:
- Around line 33-37: The Number/Boolean branch returns a Response with c.set as
init but doesn't ensure a content-type when hasSet is true; update the 'Number'
and 'Boolean' case in the switch (the code that currently returns `new
Response(${saveResponse}''+${r},c.set)` when hasSet) to merge or override
c.set.headers to include 'content-type': 'text/plain' (same as the String
branch) before passing it to new Response, so that when hasSet is true the
Response init always contains a text/plain content-type; reference the variables
hasSet, saveResponse, r, c.set, and the Response constructor in your change.
---
Nitpick comments:
In `@src/adapter/web-standard/index.ts`:
- Around line 22-41: The WebStandard specializedResponse currently sets
'content-type:text/plain' for String responses when hasSet=true, which diverges
from the BunAdapter that relies on Response.json() for objects/arrays and does
not set text/plain for strings; to align behavior, update specializedResponse
(function specializedResponse) to stop adding 'content-type:text/plain' for
kind==='String' when hasSet is true (i.e., treat String the same as
Number/Boolean: do not force a content-type), or alternatively add the explicit
text/plain header in the Bun adapter where Response is created—pick one approach
and make both adapters consistent (update the String branch in
specializedResponse and mirror the change in the BunAdapter Response creation
logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2af1589-cf9b-4af1-a7f2-a0c0bec76301
📒 Files selected for processing (1)
src/adapter/web-standard/index.ts
| case 'Number': | ||
| case 'Boolean': | ||
| return hasSet | ||
| ? `new Response(${saveResponse}''+${r},c.set)` | ||
| : `new Response(${saveResponse}''+${r})` |
There was a problem hiding this comment.
Missing content-type header for Number/Boolean when hasSet is true.
The String case (lines 30-31) sets content-type: text/plain when hasSet is true, but Number/Boolean does not. When c.set is passed as the Response init, the default content-type behavior is overridden—if c.set.headers lacks a content-type, the response will have none.
This inconsistency could cause clients to misinterpret numeric or boolean responses when headers/cookies/status are active.
🐛 Proposed fix to add content-type for Number/Boolean
case 'Number':
case 'Boolean':
return hasSet
- ? `new Response(${saveResponse}''+${r},c.set)`
+ ? `(c.set.headers['content-type']||(c.set.headers['content-type']='text/plain'),new Response(${saveResponse}''+${r},c.set))`
: `new Response(${saveResponse}''+${r})`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapter/web-standard/index.ts` around lines 33 - 37, The Number/Boolean
branch returns a Response with c.set as init but doesn't ensure a content-type
when hasSet is true; update the 'Number' and 'Boolean' case in the switch (the
code that currently returns `new Response(${saveResponse}''+${r},c.set)` when
hasSet) to merge or override c.set.headers to include 'content-type':
'text/plain' (same as the String branch) before passing it to new Response, so
that when hasSet is true the Response init always contains a text/plain
content-type; reference the variables hasSet, saveResponse, r, c.set, and the
Response constructor in your change.
|
The generic |
1 participant
Summary
When a route declares a response schema (e.g.
response: t.Object({...})), the return type is already known at compile time. Yet after validation, the response still goes through the genericmapResponse/mapCompactResponsechain which performs 10+ type checks (constructor.nameswitch,instanceoffor Response, Blob, ReadableStream, Error, Promise, etc.).This PR extends the existing Sucrose AOT compilation to the response path — generating specialized inline response code that bypasses the generic dispatch when the schema Kind is known.
What changed
getResponseSchemaKind()incompose.ts— detects TypeBox schema Kind (Object, Array, String, Number, Boolean) for single-status-code response schemasspecializedResponse()adapter method inadapter/types.ts— lets each adapter generate optimized inline code per typeResponse.json()for Object/Array (native fast path),new Response()for primitivesJSON.stringify()+ content-type header for Object/ArraymapResponseclosure incompose.ts— emits a type guard with fast path + fallback to genericWhy it helps
Safety
Every specialized path has a runtime type guard that falls back to the generic
mapResponseif the value doesn't match:Specialization is disabled when:
mapResponsehooks are present (could change type)maybeStreamis true (generators/streams)Test plan
bun test) — zero regressionsautocannon/bun benchfnLiteralto verify inlineResponse.json()appearsSummary by CodeRabbit