perf(middleware): precompose middleware chains#1475
Conversation
Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughChangesThe middleware system now supports precomposed execution chains. H3Core caches global, route, and mounted middleware pipelines while preserving dynamic behavior for overridden Middleware composition
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/middleware.test.ts`:
- Around line 68-94: Update the "~getMiddleware compat" test suite to use
describeMatrix so it runs in both web and node runtimes. Adapt the test setup to
the matrix context, creating the H3 app per test via beforeEach if needed, and
use ctx.fetch instead of app.request when required by the existing cross-runtime
test conventions.
🪄 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: a7d67c01-2701-48df-8194-f0a843ffc54c
📒 Files selected for processing (6)
src/h3.tssrc/handler.tssrc/middleware.tssrc/types/h3.tstest/bench/bundle.test.tstest/unit/middleware.test.ts
|
|
||
| describe("~getMiddleware compat", () => { | ||
| test("instance-level override provides per-event middleware (nitro pattern)", async () => { | ||
| const app = new H3().get("/test", (event) => `handler:${event.context.order}`); | ||
| const push = (name: string): Middleware => { | ||
| return (event, next) => { | ||
| event.context.order = `${event.context.order || ""}+${name}`; | ||
| return next(); | ||
| }; | ||
| }; | ||
| app.use(push("global")); | ||
| // Nitro assigns an instance-level override returning a per-event array: | ||
| // https://github.com/nitrojs/nitro/blob/main/src/build/virtual/app.ts | ||
| app["~getMiddleware"] = (event, route) => { | ||
| const middleware = [...app["~middleware"]]; | ||
| if (event.url.pathname === "/test" && route) { | ||
| middleware.push(push("extra")); | ||
| } | ||
| return middleware; | ||
| }; | ||
| // Repeated requests: override must run per event (no stale precomposition) | ||
| for (let i = 0; i < 2; i++) { | ||
| const res = await app.request("/test"); | ||
| expect(await res.text()).toBe("handler:+global+extra"); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use describeMatrix for tests instead of describe.
As per coding guidelines, tests should use describeMatrix to ensure they execute across both web and node runtimes. Additionally, consider whether the app instance should be set up via a beforeEach hook and whether to use ctx.fetch instead of app.request to fully validate cross-runtime behavior.
🤖 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 `@test/unit/middleware.test.ts` around lines 68 - 94, Update the
"~getMiddleware compat" test suite to use describeMatrix so it runs in both web
and node runtimes. Adapt the test setup to the matrix context, creating the H3
app per test via beforeEach if needed, and use ctx.fetch instead of app.request
when required by the existing cross-runtime test conventions.
Source: Coding guidelines
Co-Authored-By: Claude Fable 5 <[email protected]>
…nd mount() Co-Authored-By: Claude Fable 5 <[email protected]>
Resolve the custom `~getMiddleware` check once when building the cached dispatcher instead of on every handler call. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Follow-up for downstream adoption: nitrojs/nitro#4443 (drop the |
2 participants
🔗 Linked issue
none
📚 Description
Middleware dispatch currently pays a per-request cost that only depends on registration-time state:
~getMiddlewarere-builds the middleware array on every request (an array spread whenever the route has its own middleware), andcallMiddlewarewalks it recursively with index bookkeeping through a single megamorphicfn(event, next)call site.Since both the global middleware list and each route's middleware are fixed between registrations, this PR precomposes the chains once and caches them:
composeMiddleware(middleware)builds the chain right-to-left into a single(event, handler) => resultfunction. Each layer is its own closure, so middleware call sites become monomorphic. The terminal handler is threaded through as an argument (rather than baked in or read fromevent.context.matchedRoute, which a nested app can overwrite mid-chain), so one composed global chain serves every route.~composed). Only route-local state is baked in, so the cache stays valid whenmountcopies route objects.~composed) and explicitly invalidated byuse()andmount(), so middleware added after the first request is picked up. Themountwrapper reuses the mounted app's own~composedcache, so the child'suse()invalidates it too.defineHandler({ middleware })now composes once at definition time instead of re-dispatching per call.Public API
composeMiddleware(and theComposedMiddlewaretype) is exported fromh3so downstream frameworks (e.g. nitro) can adopt precomposition for their own middleware chains instead ofcallMiddleware.callMiddlewareremains exported and unchanged.Backward compatibility (
~getMiddleware)Existing users extending
H3Core(e.g. nitro) override~getMiddleware— including as an instance-level assignment — to provide per-event middleware, which cannot be precomposed. The dispatch strategy is resolved once per app (~dispatch, built on first request and invalidated byuse()/mount()): if~getMiddlewareis overridden (method identity check, run only at dispatcher build time — no per-request cost), dispatch falls back to the previouscallMiddlewarebehavior unchanged. Overrides must be in place before the first request (nitro assigns at build time). A regression test covers the nitro pattern.next()memoization semantics (undefined/kNotFoundfall-through, thenable handling) are preserved byte-for-byte; the shared per-layer logic lives in onecallLayerhelper used by both the composed chain and the publiccallMiddleware.Numbers
Isolated dispatch bench (mitata, node 24):
callMiddlewarecallMiddlewareApp-level
pnpm bench:nodeis unchanged (dispatch is sub-µs of a ~31µs iteration dominated by Request/URL work); the win scales with middleware depth and route-middleware usage.Bundle cost of shipping both the composer and the compat path: H3 +599 B (+191 B gzip), H3Core +539 B (+183 B gzip), defineHandler +103 B (+50 B gzip); budgets in
bundle.test.tsbumped to match (kept tight).Known edge (outside documented API): mutating a middleware array in place (a route's
middlewareafter its first match, or~middlewaredirectly withoutuse()) is not picked up live — invalidation is explicit viause()/mount(). Custom~getMiddlewareoverrides are unaffected.Possible follow-up: in the
h3-middlewarebench scenario the remaining overhead is dominated bynormalizeMiddleware's per-request route regexp matching and themiddlewareParamsspread — baking matchers into composition would be the next lever.🤖 Generated with Claude Code
Summary by CodeRabbit
composeMiddleware/composeHandler) and re-exportedComposedMiddleware.use()andmount().