Conversation
Added typed context to h3, so that we can have better type inference for the context object in handlers.
📝 WalkthroughWalkthroughThe PR makes the H3 core context-aware by introducing a generic context type parameter across ChangesContext-Aware Generic Type System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/h3.ts (1)
232-232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the generic type parameter in the exported
H3constructor.The construct signature is missing the generic type parameter, preventing callers from writing
new H3<MyContext>()even though the class is implemented as generic. Add<_ContextT extends H3EventContext = H3EventContext>to the construct signature so consumers can leverage the per-app typing this PR introduces.Suggested fix
-})() as unknown as { new (config?: H3Config): H3Type }; +})() as unknown as { + new <_ContextT extends H3EventContext = H3EventContext>( + config?: H3Config, + ): H3Type<_ContextT>; +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/h3.ts` at line 232, The exported constructor cast currently drops the generic parameter — update the construct signature cast on the IIFE result so it preserves the generic type parameter; change the cast of the IIFE to include "<_ContextT extends H3EventContext = H3EventContext>" on the new signature and ensure the returned type uses H3Type<_ContextT>, keeping H3Config as the constructor arg type; adjust the as unknown as { new (...) : H3Type } expression to include these generic bounds so callers can write new H3<MyContext>().
🧹 Nitpick comments (1)
src/types/handler.ts (1)
7-7: ⚡ Quick winImport
H3EventContextfrom its concrete module, not the barrel.
../index.tsreintroduces a barrel import in a type-only module that already imports peers directly.Suggested fix
-import type { H3EventContext } from "../index.ts"; +import type { H3EventContext } from "./context.ts";As per coding guidelines, "Do not use barrel files — import directly from specific modules".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/handler.ts` at line 7, Replace the barrel import of H3EventContext in src/types/handler.ts with a direct import from the concrete module that exports H3EventContext (do not import from "../index.ts"); locate the import line importing H3EventContext and change it to import from the specific module that defines H3EventContext so the file imports the type directly rather than through the barrel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/types/h3.ts`:
- Around line 128-151: The request() context parameter is typed as the full
augmented _ContextT (including middleware-added keys from addEventContext),
forcing callers to provide runtime-populated keys; change request() to accept
only the seed context (i.e. _ContextT without middleware-added keys).
Concretely, update the H3 type signatures so addEventContext() continues to
return H3<_ContextT & Record<K,V>> but request(request, options?, context?) uses
a seed type (either introduce a separate generic like _SeedT or use
Omit<_ContextT, keyof MiddlewareAddedKeys>) so callers pass only the initial
context; keep event.context typing as the augmented type used during middleware
handling. Ensure to adjust the request(...) signature and related overloads to
reference the new seed/omitted type while leaving addEventContext, H3, and event
types unchanged otherwise.
- Around line 156-160: Route-level middleware loses context because RouteOptions
is non-generic and uses Middleware[]; make RouteOptions generic (e.g.,
RouteOptions<_ContextT>) and change its middleware property to
Middleware<_ContextT>[] so middleware can access the route's context. Thread
this generic through all routing APIs: update on(), all(), and verb helpers
(get, post, put, delete, patch, head, options, connect, trace) to accept opts?:
RouteOptions<_ContextT>, and adjust H3Route (or internal route record types) to
use H3Route<_ContextT> where routes store opts/handler so everything preserves
the _ContextT generic from HTTPHandler<_ContextT> to middleware.
In `@src/types/handler.ts`:
- Around line 25-33: EventHandlerObject currently defines middleware?:
Middleware[] which drops the generic _ContextT and prevents middleware from
inferring the custom context; update the type to propagate the context by
changing middleware?: Middleware[] to middleware?: Middleware<_ContextT>[] (and
adjust any related type imports/usages) so middleware functions receive the same
_ContextT as handler and restore proper type inference for context-aware
middleware.
---
Outside diff comments:
In `@src/h3.ts`:
- Line 232: The exported constructor cast currently drops the generic parameter
— update the construct signature cast on the IIFE result so it preserves the
generic type parameter; change the cast of the IIFE to include "<_ContextT
extends H3EventContext = H3EventContext>" on the new signature and ensure the
returned type uses H3Type<_ContextT>, keeping H3Config as the constructor arg
type; adjust the as unknown as { new (...) : H3Type } expression to include
these generic bounds so callers can write new H3<MyContext>().
---
Nitpick comments:
In `@src/types/handler.ts`:
- Line 7: Replace the barrel import of H3EventContext in src/types/handler.ts
with a direct import from the concrete module that exports H3EventContext (do
not import from "../index.ts"); locate the import line importing H3EventContext
and change it to import from the specific module that defines H3EventContext so
the file imports the type directly rather than through the barrel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0200294-bad2-4c28-89fb-71723cd0bba9
📒 Files selected for processing (4)
src/event.tssrc/h3.tssrc/types/h3.tssrc/types/handler.ts
Comment on lines
+128
to
+151
| request( | ||
| request: ServerRequest | URL | string, | ||
| options?: RequestInit, | ||
| context?: H3EventContext, | ||
| context?: _ContextT, | ||
| ): Response | Promise<Response>; | ||
|
|
||
| /** | ||
| * Register a global middleware. | ||
| */ | ||
| use(route: string, handler: Middleware | H3, opts?: MiddlewareOptions): this; | ||
| use(handler: Middleware | H3, opts?: MiddlewareOptions): this; | ||
| use( | ||
| route: string, | ||
| handler: Middleware<_ContextT> | H3<_ContextT>, | ||
| opts?: MiddlewareOptions, | ||
| ): this; | ||
| use(handler: Middleware<_ContextT> | H3<_ContextT>, opts?: MiddlewareOptions): this; | ||
|
|
||
| /** | ||
| * Register a key in the event context | ||
| */ | ||
| addEventContext<K extends string, V>( | ||
| key: K, | ||
| valFn: (event: H3Event<EventHandlerRequest, _ContextT>) => V, | ||
| ): H3<_ContextT & Record<K, V>>; | ||
| addEventContext<K extends string, V>(key: K, val: V): H3<_ContextT & Record<K, V>>; |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Inspect the public request/addEventContext signatures together.
sed -n '117,151p' src/types/h3.tsRepository: h3js/h3
Repository: h3js/h3
Exit code: 0
stdout:
export declare class H3<_ContextT extends H3EventContext = H3EventContext> extends H3Core {
/** `@internal` */
"~rou3": RouterContext;
/**
* A [fetch](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)-compatible API allowing to fetch app routes.
*
* Input can be a URL, relative path or standard [Request](https://developer.mozilla.org/en-US/docs/Web/API/Request) object.
*
* Returned value is a [Response](https://developer.mozilla.org/en-US/docs/Web/API/Response) Promise.
*/
request(
request: ServerRequest | URL | string,
options?: RequestInit,
context?: _ContextT,
): Response | Promise<Response>;
/**
* Register a global middleware.
*/
use(
route: string,
handler: Middleware<_ContextT> | H3<_ContextT>,
opts?: MiddlewareOptions,
): this;
use(handler: Middleware<_ContextT> | H3<_ContextT>, opts?: MiddlewareOptions): this;
/**
* Register a key in the event context
*/
addEventContext<K extends string, V>(
key: K,
valFn: (event: H3Event<EventHandlerRequest, _ContextT>) => V,
): H3<_ContextT & Record<K, V>>;
addEventContext<K extends string, V>(key: K, val: V): H3<_ContextT & Record<K, V>>;
Script executed:
# Find and inspect the request() method implementation
fd -t f "\.ts$" src | head -20Repository: h3js/h3
Repository: h3js/h3
Exit code: 0
stdout:
src/_deprecated.ts
src/_entries/_common.ts
src/_entries/bun.ts
src/_entries/cloudflare.ts
src/_entries/deno.ts
src/_entries/generic.ts
src/_entries/node.ts
src/_entries/service-worker.ts
src/adapters.ts
src/error.ts
src/event.ts
src/h3.ts
src/handler.ts
src/index.ts
src/middleware.ts
src/plugin.ts
src/response.ts
src/tracing.ts
src/types/_utils.ts
src/types/context.ts
Script executed:
# Search for the implementation of the request method
rg -A 15 "request\(" src --type ts | grep -A 15 "request(request:"Repository: h3js/h3
Repository: h3js/h3
Exit code: 1
Script executed:
# Look for tests or examples using addEventContext and request together
rg "addEventContext" --type ts -B 2 -A 5Repository: h3js/h3
Repository: h3js/h3
Exit code: 0
stdout:
src/types/h3.ts- * Register a key in the event context
src/types/h3.ts- */
src/types/h3.ts: addEventContext<K extends string, V>(
src/types/h3.ts- key: K,
src/types/h3.ts- valFn: (event: H3Event<EventHandlerRequest, _ContextT>) => V,
src/types/h3.ts- ): H3<_ContextT & Record<K, V>>;
src/types/h3.ts: addEventContext<K extends string, V>(key: K, val: V): H3<_ContextT & Record<K, V>>;
src/types/h3.ts-
src/types/h3.ts- /**
src/types/h3.ts- * Register a route handler for the specified HTTP method and route.
src/types/h3.ts- */
src/types/h3.ts- on(
--
src/h3.ts- }
src/h3.ts-
src/h3.ts: addEventContext<K extends string, V>(
src/h3.ts- key: K,
src/h3.ts- valOrFn: unknown,
src/h3.ts- ): H3Type<_ContextT & Record<K, V>> {
src/h3.ts- this.use(((event) => {
src/h3.ts- event.context[key] = typeof valOrFn === "function" ? valOrFn(event) : valOrFn;
Script executed:
# Check how context parameter is actually used in request implementation
rg "context\?" src --type ts -B 2 -A 2 | head -100Repository: h3js/h3
Repository: h3js/h3
Exit code: 0
stdout:
src/types/h3.ts-
src/types/h3.ts- /** `@internal` */
src/types/h3.ts: "~request"(request: ServerRequest, context?: H3EventContext): Response | Promise<Response>;
src/types/h3.ts-
src/types/h3.ts- /** `@internal` */
--
src/types/h3.ts- request: ServerRequest | URL | string,
src/types/h3.ts- options?: RequestInit,
src/types/h3.ts: context?: _ContextT,
src/types/h3.ts- ): Response | Promise<Response>;
src/types/h3.ts-
--
src/utils/session.ts- get id() {
src/utils/session.ts- const context = getEventContext<H3EventContext>(event);
src/utils/session.ts: return context?.sessions?.[sessionName]?.id;
src/utils/session.ts- },
src/utils/session.ts- get data() {
--
src/utils/request.ts- }
src/utils/request.ts-
src/utils/request.ts: return (event.req.context?.clientAddress as string) || event.req.ip || undefined;
src/utils/request.ts-}
--
src/h3.ts- }
src/h3.ts-
src/h3.ts: "~request"(request: ServerRequest, context?: H3EventContext): Response | Promise<Response> {
src/h3.ts- // Create a new event instance
src/h3.ts- const event = new H3Event(request, context, this as unknown as H3Type);
--
src/h3.ts- _req: ServerRequest | URL | string,
src/h3.ts- _init?: RequestInit,
src/h3.ts: context?: _ContextT,
src/h3.ts- ): Response | Promise<Response> {
src/h3.ts- return this["~request"](toRequest(_req, _init), context);
--
src/adapters.ts-
src/adapters.ts-/**
src/adapters.ts: * `@deprecated` Since h3 v2 you can directly use `app.fetch(request, init?, context?)`
src/adapters.ts- */
src/adapters.ts-export function toWebHandler(
src/adapters.ts- app: H3,
src/adapters.ts:): (request: ServerRequest, context?: H3EventContext) => Promise<Response> {
src/adapters.ts- return (request, context) => {
src/adapters.ts- return Promise.resolve(app.request(request, undefined, context || request.context));
--
src/adapters.ts-
src/adapters.ts-export function fromWebHandler(
src/adapters.ts: handler: (request: ServerRequest, context?: H3EventContext) => Promise<Response>,
src/adapters.ts-): EventHandler {
src/adapters.ts- return function _webHandler(event) {
--
src/event.ts- static __is_event__ = true;
src/event.ts-
src/event.ts: constructor(req: ServerRequest, context?: _ContextT, app?: H3Core) {
src/event.ts- this.context = context || req.context || new EmptyObject();
src/event.ts- this.req = req;
request() context parameter should not include keys added by addEventContext() middleware.
The type signature currently requires that context parameter matches the full augmented type (_ContextT & Record<K, V>), but middleware-added keys are populated at runtime during request handling, not supplied as seed context by the caller. This breaks type safety for applications that use addEventContext().
Example:
const app = new H3<{ traceId: string }>()
.addEventContext("auth", (event) => computeAuth(event));
// Type requires both traceId and auth, but auth is middleware-populated
app.request("/x", undefined, { traceId: "abc" }); // ❌ Type error: auth missingThe request() method should accept only the seed context type (_ContextT without middleware-added keys), while event.context is correctly enhanced during middleware execution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/h3.ts` around lines 128 - 151, The request() context parameter is
typed as the full augmented _ContextT (including middleware-added keys from
addEventContext), forcing callers to provide runtime-populated keys; change
request() to accept only the seed context (i.e. _ContextT without
middleware-added keys). Concretely, update the H3 type signatures so
addEventContext() continues to return H3<_ContextT & Record<K,V>> but
request(request, options?, context?) uses a seed type (either introduce a
separate generic like _SeedT or use Omit<_ContextT, keyof MiddlewareAddedKeys>)
so callers pass only the initial context; keep event.context typing as the
augmented type used during middleware handling. Ensure to adjust the
request(...) signature and related overloads to reference the new seed/omitted
type while leaving addEventContext, H3, and event types unchanged otherwise.
Comment on lines
156
to
160
| on( | ||
| method: HTTPMethod | Lowercase<HTTPMethod> | "", | ||
| route: string, | ||
| handler: HTTPHandler, | ||
| handler: HTTPHandler<_ContextT>, | ||
| opts?: RouteOptions, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the route options definition and the updated route-registration signatures.
sed -n '63,66p' src/types/h3.ts
sed -n '153,190p' src/types/h3.tsRepository: h3js/h3
Length of output: 1835
🏁 Script executed:
#!/bin/bash
# Find the Middleware type definition and related context types
rg "export type Middleware" src/types/ -A 3
rg "export type HTTPHandler" src/types/ -A 3
rg "export.*H3EventContext" src/types/ -A 2Repository: h3js/h3
Length of output: 1638
🏁 Script executed:
#!/bin/bash
# Check all usages of RouteOptions and how it's threaded through the codebase
rg "RouteOptions" src/types/ -B 2 -A 1
rg "RouteOptions" src/ --include="*.ts" | grep -v test | head -30Repository: h3js/h3
Length of output: 1936
🏁 Script executed:
#!/bin/bash
# Check H3Route definition and implementation details
rg "export.*type H3Route" src/types/ -A 5
rg "class H3" src/ -t ts -A 10 | head -40Repository: h3js/h3
Length of output: 1462
🏁 Script executed:
#!/bin/bash
# Look for H3Route type definition
rg "type H3Route" src/types/ -B 2 -A 5Repository: h3js/h3
Length of output: 33
🏁 Script executed:
#!/bin/bash
# Search for H3Route definition more broadly
rg "H3Route" src/types/ -B 1 -A 3
rg "interface H3Route" src/ -t ts -A 5Repository: h3js/h3
Length of output: 2954
🏁 Script executed:
#!/bin/bash
# Check EventHandler definition
rg "export type EventHandler" src/types/ -A 3
# Check the actual implementation of on() method
sed -n '100,250p' src/h3.ts | head -80Repository: h3js/h3
Length of output: 3357
🏁 Script executed:
#!/bin/bash
# Look at more of the on() implementation and check all verb methods
sed -n '100,160p' src/h3.ts | cat -n
# Also check what HTTPHandler signature expects
rg "export type HTTPHandler" src/types/handler.ts -A 5Repository: h3js/h3
Length of output: 2646
Route-level middleware loses the typed context.
on() and all verb helpers accept handler: HTTPHandler<_ContextT> for context awareness, but opts remains plain RouteOptions with middleware?: Middleware[]. Since Middleware is already generic (Middleware<_ContextT>), route-level middleware cannot see the added context keys.
Make RouteOptions generic and thread it through all routing methods:
Suggested direction
-export type RouteOptions = {
- middleware?: Middleware[];
+export type RouteOptions<_ContextT extends H3EventContext = H3EventContext> = {
+ middleware?: Middleware<_ContextT>[];
meta?: H3RouteMeta;
};Then update on(), all(), and the verb helpers (get, post, put, delete, patch, head, options, connect, trace) to accept opts?: RouteOptions<_ContextT>. Mirror this on H3Route if needed for internal route records.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/h3.ts` around lines 156 - 160, Route-level middleware loses context
because RouteOptions is non-generic and uses Middleware[]; make RouteOptions
generic (e.g., RouteOptions<_ContextT>) and change its middleware property to
Middleware<_ContextT>[] so middleware can access the route's context. Thread
this generic through all routing APIs: update on(), all(), and verb helpers
(get, post, put, delete, patch, head, options, connect, trace) to accept opts?:
RouteOptions<_ContextT>, and adjust H3Route (or internal route record types) to
use H3Route<_ContextT> where routes store opts/handler so everything preserves
the _ContextT generic from HTTPHandler<_ContextT> to middleware.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/types/h3.ts (2)
128-132:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
request()contextshould be the seed context, not the augmented one.
context?: _ContextTrequires callers to supply the full augmented type that includes keys populated at runtime byextendContext-installed middleware. After chainingextendContext,_ContextTbecomes_ContextT & Record<K, V>, soapp.request(url, init, { /* seed only */ })will fail type checking unless the caller passes the runtime-populated keys too.Consider tracking "seed" and "augmented" contexts as two distinct generics on
H3— e.g.H3<_SeedT, _ContextT = _SeedT>— whereextendContextonly widens_ContextT, whilerequest(... , context?: _SeedT)keeps using the seed type.event.context(typed via_ContextT) remains the augmented shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/h3.ts` around lines 128 - 132, The request(...) signature currently types the third parameter as the augmented _ContextT, forcing callers to supply runtime-added keys; change the type model to distinguish seed vs augmented contexts by introducing two generics on H3 (e.g. H3<_SeedT, _ContextT = _SeedT>), have extendContext widen only the _ContextT generic while keeping request(..., context?: _SeedT) typed with the seed context, and ensure event.context remains _ContextT (augmented) so runtime middleware keys are preserved for handlers but not required when calling request; update the request declaration, extendContext signature, and any H3 instantiations to follow this two-generic pattern to fix the type mismatch.
63-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RouteOptionsis generic but not threaded through routing methods.
RouteOptions<_ContextT>was made generic, buton,all, and the verb helpers still declareopts?: RouteOptions(defaulting toH3EventContext). As a result, route-level middleware passed viaopts.middlewarefalls back to the baseMiddleware<H3EventContext>and cannot see keys added byextendContext, which is exactly what the generic was introduced to enable.on( method: HTTPMethod | Lowercase<HTTPMethod> | "", route: string, handler: HTTPHandler<_ContextT>, - opts?: RouteOptions, + opts?: RouteOptions<_ContextT>, ): this; - all(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - get(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - post(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - put(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - delete(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - patch(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - head(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - options(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - connect(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; - trace(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions): this; + all(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + get(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + post(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + put(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + delete(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + patch(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + head(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + options(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + connect(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this; + trace(route: string, handler: HTTPHandler<_ContextT>, opts?: RouteOptions<_ContextT>): this;Also applies to: 157-162, 181-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/h3.ts` around lines 63 - 66, RouteOptions is generic but its type parameter isn't threaded into routing APIs, so change the signatures of functions like on, all, and all HTTP verb helpers that accept opts?: RouteOptions to accept opts?: RouteOptions<_ContextT> (or a matching generic param) so route-level middleware uses Middleware<_ContextT> instead of Middleware<H3EventContext>; update the corresponding function/method generic declarations where you see opts?: RouteOptions to propagate the same _ContextT generic (and adjust callers/exports) and ensure extendContext's augmented context type is used by middleware typed as Middleware<_ContextT>.
🧹 Nitpick comments (1)
src/types/h3.ts (1)
144-152: 💤 Low value
extendContextvalue overload +typeof === "function"runtime check.The implementation in
src/h3.ts(line 210) treats anyvalOrFnwhosetypeofis"function"as a factory. The value overloadextendContext<K, V>(key: K, val: V)therefore silently turns a callableV(e.g. a class, bound method, or stringified-handler value) into a factory invocation. This isn't reflected in the type system. Consider either splitting into two methods (extendContextfor static values,extendContextFactory/extendContextWithfor the function form) or adding a{ value: V } | { factory: (...) => V }shape so intent is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/h3.ts` around lines 144 - 152, The current extendContext overloads conflate static values and factory functions causing callable values to be treated as factories; update the API to a discriminated-union form and adjust the impl instead of using typeof checks: change the extendContext signatures (and call sites) to accept either { value: V } or { factory: (event: H3Event<EventHandlerRequest, _ContextT>) => V } (e.g., extendContext<K extends string, V>(key: K, opt: { value: V } | { factory: (event: H3Event<...>) => V }): H3<_ContextT & Record<K, V>>), then modify the implementation (the function currently inspecting valOrFn / valFn) to branch on the discriminant (e.g., 'factory' in opt) and only call the factory when provided; update any callers to pass the correct shape so callable values are preserved as static values unless explicitly wrapped as a factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/h3.ts`:
- Around line 208-212: The extendContext implementation currently mutates the
instance with this.use(...) but its signature/JSDoc promise a new H3<_ContextT &
Record<K,V>> instance and also treats any function-valued valOrFn as a factory;
fix by either (A) changing extendContext to create and return a new H3 instance
that clones current routes/middleware and registers the new middleware so only
subsequently-added handlers see the new context (update JSDoc and preserve the
H3<_ContextT & Record<K,V>> return type), or (B) keep returning this but change
the TypeScript signature and JSDoc to return this and drop the widened context
type, documenting the global effect; additionally disambiguate valOrFn by using
an overload or explicit predicate (not plain typeof) so a user-supplied function
value isn’t unintentionally invoked (refer to extendContext, use, Middleware,
H3, and event.context when making these edits).
---
Duplicate comments:
In `@src/types/h3.ts`:
- Around line 128-132: The request(...) signature currently types the third
parameter as the augmented _ContextT, forcing callers to supply runtime-added
keys; change the type model to distinguish seed vs augmented contexts by
introducing two generics on H3 (e.g. H3<_SeedT, _ContextT = _SeedT>), have
extendContext widen only the _ContextT generic while keeping request(...,
context?: _SeedT) typed with the seed context, and ensure event.context remains
_ContextT (augmented) so runtime middleware keys are preserved for handlers but
not required when calling request; update the request declaration, extendContext
signature, and any H3 instantiations to follow this two-generic pattern to fix
the type mismatch.
- Around line 63-66: RouteOptions is generic but its type parameter isn't
threaded into routing APIs, so change the signatures of functions like on, all,
and all HTTP verb helpers that accept opts?: RouteOptions to accept opts?:
RouteOptions<_ContextT> (or a matching generic param) so route-level middleware
uses Middleware<_ContextT> instead of Middleware<H3EventContext>; update the
corresponding function/method generic declarations where you see opts?:
RouteOptions to propagate the same _ContextT generic (and adjust
callers/exports) and ensure extendContext's augmented context type is used by
middleware typed as Middleware<_ContextT>.
---
Nitpick comments:
In `@src/types/h3.ts`:
- Around line 144-152: The current extendContext overloads conflate static
values and factory functions causing callable values to be treated as factories;
update the API to a discriminated-union form and adjust the impl instead of
using typeof checks: change the extendContext signatures (and call sites) to
accept either { value: V } or { factory: (event: H3Event<EventHandlerRequest,
_ContextT>) => V } (e.g., extendContext<K extends string, V>(key: K, opt: {
value: V } | { factory: (event: H3Event<...>) => V }): H3<_ContextT & Record<K,
V>>), then modify the implementation (the function currently inspecting valOrFn
/ valFn) to branch on the discriminant (e.g., 'factory' in opt) and only call
the factory when provided; update any callers to pass the correct shape so
callable values are preserved as static values unless explicitly wrapped as a
factory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bf9e14a-9faf-415d-bbb4-a953b0d4160b
📒 Files selected for processing (3)
src/h3.tssrc/types/h3.tssrc/types/handler.ts
Comment on lines
+208
to
+212
| extendContext(key: string, valOrFn: unknown): this { | ||
| return this.use(((event) => { | ||
| event.context[key] = typeof valOrFn === "function" ? valOrFn(event) : valOrFn; | ||
| }) as Middleware); | ||
| } |
There was a problem hiding this comment.
extendContext fluent typing doesn't match runtime scoping.
The declared signature in src/types/h3.ts returns H3<_ContextT & Record<K, V>> (and the JSDoc on line 146 there says it returns "a new H3 instance"), implying that only handlers/middleware registered after the call see the extra key. The implementation, however, just calls this.use(...) and returns this, so the appended middleware runs for every route on the instance — including ones registered before extendContext. Earlier routes get the key at runtime but their event.context is typed without it, while the original H3<_ContextT> reference still being held by callers becomes unsound.
Two related concerns:
- The typing should either be honored by returning a new instance (or scoping the middleware to subsequently-added routes), or the JSDoc should be corrected and the return type should remain
this(H3<_ContextT & Record<K, V>>cast aside) with a documented caveat. valOrFn: unknown+typeof valOrFn === "function"means a caller using the value-overload with a function value will have it invoked as a factory. Consider distinguishing by overload at the implementation level (e.g., separate methods, or anisFnflag) if that case matters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/h3.ts` around lines 208 - 212, The extendContext implementation currently
mutates the instance with this.use(...) but its signature/JSDoc promise a new
H3<_ContextT & Record<K,V>> instance and also treats any function-valued valOrFn
as a factory; fix by either (A) changing extendContext to create and return a
new H3 instance that clones current routes/middleware and registers the new
middleware so only subsequently-added handlers see the new context (update JSDoc
and preserve the H3<_ContextT & Record<K,V>> return type), or (B) keep returning
this but change the TypeScript signature and JSDoc to return this and drop the
widened context type, documenting the global effect; additionally disambiguate
valOrFn by using an overload or explicit predicate (not plain typeof) so a
user-supplied function value isn’t unintentionally invoked (refer to
extendContext, use, Middleware, H3, and event.context when making these edits).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added typed context to h3, so that we can have better type inference for the context object in handlers.
contexthas been made generic inH3EventandH3addEventContexthas been added toH3for attaching a typed context.Code like this is type-safe now-
Feedback is welcome!
resolves #511
Summary by CodeRabbit