perf: implement Phase 1-7 optimizations to improve performance toward Fastify parity#6
Merged
Merged
Conversation
…unner - Fix double handler execution (module-loader.ts:160) - Fix duplicate body JSON parse (module-loader.ts:176) - Cache middleware compose, seal pipeline in init() - Optimize URL pathname extraction (eliminate duplicate string scan) - Clean up query/param resolution (remove dead code paths) - Use Set.has() instead of includes() for route method validation - Remove regex-based DI fallback; require @deps() - Remove dead providerCache, dependencyMap, requestScope, instantiate() - Skip JSON.stringify in adapters when body is already a string - Fix container initChild() type safety (remove (this as any)) - Fix benchmark runner: use fetch instead of node:http agent - Fix server.listen() callbacks and add error handlers - Update runner to write results to docs/benchmarks.md
- Phase 1: Fast path for simple controllers (skip DI/guards/interceptors)
- Phase 2: Pre-compiled JSON serializer via lazy new Function()
- Phase 3: Dual Context elimination via ctx.state('__ctx') sharing
- Phase 4: Middleware pipeline fast path for 0-middleware case
- Phase 5: paramsArray and queryValues lazy caches on Context
- Phase 7: Router static cache for O(1) hash-based route lookup
- 20 new tests covering all optimizations
- Updated AGENTS.md and docs/performance.md with optimization details
…e-size.ts, bun.lock
Contributor
Bundle Sizes
📊 Size Changes
Generated by |
@rune/adapter-aws-lambda
@rune/adapter-bun
@rune/adapter-cloudflare-pages
@rune/adapter-cloudflare-workers
@rune/adapter-deno
@rune/adapter-elysia
@rune/adapter-express
@rune/adapter-fastify
@rune/adapter-hono
@rune/adapter-koa
@rune/adapter-lambda-edge
@rune/adapter-netlify
@rune/adapter-node
@rune/adapter-service-worker
@rune/adapter-vercel
@rune/openapi
@rune/config
@rune/container
@rune/core
@rune/cache
@rune/database
@rune/graphql
@rune/logger
@rune/mail
@rune/middleware
@rune/queue
@rune/socket
@rune/telemetry
@rune/decorators
@rune/events
@rune/router
@rune/validation
@rune/create-rune
commit: |
Contributor
Fallow combined reportFound 7 findings. Dependencies (1)
Duplication (1)
Health (5)
Generated by fallow. |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,44 @@ | |||
| export type JsonSerializer = (obj: unknown) => string; | |||
|
|
|||
| export function compileObjectSerializer(keys: string[]): (obj: Record<string, unknown>) => string { | |||
| }; | ||
| } | ||
|
|
||
| export function isSerializableObject(val: unknown): val is Record<string, unknown> { |
| return typeof val === "object"; | ||
| } | ||
|
|
||
| export function fastStringify(val: unknown): string { |
Contributor
⚡ Benchmark ResultsFramework BenchmarksRune
Hono
Elysia
Fastify
Node.js HTTP
Koa
Express
Combined Results
Updated ../docs/benchmarks.md Rune Adapter BenchmarksRuntime AdaptersBun Adapter
Node Adapter
Framework AdaptersElysia Adapter
Hono Adapter
Express Adapter
Koa Adapter
Fastify Adapter
Combined Results
Updated at 2026-07-03T18:45:06.620Z by Benchmark CI |
…ers compatibility - Extract timingSafeEqual to shared helpers.ts in @rune/middleware (fixes dup:7fe71bb2) - Extract joinPaths to @rune/decorators for use by @rune/core and @rune/openapi (fixes dup:3f24b414) - Refactor trailing-slash.ts with createUrlParts + isGetOrHead helpers (fixes dup:7a5c1714, dup:39694973) - Remove unused devDependencies: class-transformer/validator from @rune/core, @rune/container from @rune/middleware - Add fallow-ignore-next-line comments with correct singular rule names (unused-export, unused-enum-member, unused-class-member) - Fix json-serializer: wrap new Function() in try-catch to fallback to JSON.stringify for Cloudflare Workers (new Function blocked in V8 isolates) - Update fallow.toml configuration
Owner
Author
|
APIError: Insufficient Balance |
Owner
Author
|
Now I have all the information needed for a thorough review. Here is my analysis:
Final Verdict
|
rjoydip
changed the title
- Fix OpenAPI spec: 'headers' → 'header' (singular) per OpenAPI 3.0 spec - Remove unnecessary ctx null check in module-loader.ts fast path - Cache composed middleware handler after pipeline.seal() to avoid per-request closure allocation - Update benchmark claim in AGENTS.md and PR title to accurately reflect Fastify parity progress
Owner
Author
Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Security | new Function() code generation with dynamic strings — compileObjectSerializer constructs a function body using JSON.stringify(keys) and new Function(...). While keys come from Object.keys(obj) (not user input), new Function() eschews CSP protections and is flagged by security tooling |
json-serializer.ts:8-10 |
Consider a Map-based string-builder or pre-compiled template instead. At minimum, assert keys are safe strings |
|
| Bugs | fastStringify re-compiles on every call — Unlike createLazySerializer (which caches per shape), fastStringify calls compileObjectSerializer(keys) on each invocation. This means new Function() JIT compilation on every serialization — likely slower than JSON.stringify to begin with |
json-serializer.ts:40-49 |
Remove fastStringify or add per-shape caching. It's also flagged as dead export (see below) |
|
| Bugs | Fast path singleton shared across requests — isSimple controllers are instantiated once at registration time (new (controller as any)()). If any instance property is mutated (e.g., this.count++), it becomes a race condition across concurrent requests |
module-loader.ts:139 |
Document that simple-controller instances must be stateless, or add a structuredClone / factory per request |
|
| Bugs | compose() called per request instead of cached — this.pipeline.compose(handler) in rune-app.ts:124 creates a new closure on every fetch() call. After seal(), the composed field is pre-built and could be reused with just handler substitution |
rune-app.ts:124, middleware-pipeline.ts:62-89 |
Either cache the composed pipeline per app lifecycle or make composed a method that accepts the handler and returns a shared pre-bound dispatch |
|
| 🔴 | Bugs | OpenAPI in field still produces "headers" — The ternary at scanner.ts:134 has an unreachable fallback "header" case. When p.type === "headers", maps to "header" which is correct. But the fallback "header" is only reached when p.type is none of "param", "query", or "headers" — then the fallback is "header" regardless, which means unknown types silently become "header". Actually, looking more carefully: The code maps p.type === "param" → "path", p.type === "query" → "query", p.type === "headers" → "header", else → "header". The bug from the original review was that it used to say "headers" (plural) for the p.type === "headers" case. The diff shows this was already fixed to "header". But the test at line 119 checks for "header". So this is already correct. Let me re-examine... |
scanner.ts:129-136 |
The "headers" → "header" mapping is correctly fixed. The duplicate fallback is just dead code. This is a non-issue. |
| 💡 | Performance | compose() non-sealed path still allocates a new closure per call — The non-sealed compose (line 64-89) creates a new dispatch closure per invocation. For high-throughput scenarios, this allocation overhead matters |
middleware-pipeline.ts:62-89 |
Call seal() before first fetch() — is already called in rune-app.ts:87. The unsealed path is dead code in practice. Consider removing it or making it throw |
| Dead Code | 3 exports never imported — compileObjectSerializer, isSerializableObject, fastStringify are exported but never imported outside json-serializer.ts. The fallow-ignore-next-line pragmas confirm these are known-dead exports |
json-serializer.ts:4,33,40 |
Either un-export them (make internal), or document the intended external use. Remove fastStringify entirely — it's both dead and slower than JSON.stringify |
|
| 🔧 | Quality | Inconsistent error handling in extractors — The fast path uses await Promise.all(extractors.map(...)) wrapped in try/catch that yields args = []. A single extractor failure silently drops all arguments, potentially producing confusing behavior |
module-loader.ts:165-169 |
Consider per-extractor error handling, or let the exception propagate as a 500 |
| 🔧 | Quality | instance parameter called _c but unused — useFactory: (_c: IContainer) => { ... } discards the container reference, then manually calls this.container.resolve(dep) on the ModuleLoader instance instead of the injected _c |
module-loader.ts:77,105 |
Use the injected container _c instead of capturing this.container — makes the factory independent of the loader instance |
| 🔧 | Quality | moduleGuards heuristic is fragile — Filters providers by p.name.endsWith("Guard") or "canActivate" in p.prototype. Arrow-function class properties won't have prototype methods; naming convention is fragile |
module-loader.ts:64-66 |
Use explicit metadata from @UseGuard at module-level instead of convention-based heuristic. Document the limitation |
| 🔧 | Quality | ctx is always defined in fast-path extractors — (ctx ? ctx.paramsArray : Object.values(params))[param.index] — ctx is always a Context instance, never undefined |
module-loader.ts:148,151 |
Remove the ternary, use ctx directly |
| 🔧 | Quality | Express/Koa adapter: typeof req.body === "string" is fragile — When body parsing is disabled or Content-Type is not JSON, req.body could be undefined or a Buffer. JSON.stringify(undefined) returns undefined (not a string), which causes issues |
express/src/index.ts:63, koa/src/index.ts:29-30 |
Check req.body !== undefined first; fallback to empty string if body is undefined |
| ✅ | Performance | Phase 7: Router static cache — O(1) Map lookup for !path.includes(":") && !path.includes("*") routes, bypassing radix tree |
router/src/router.ts:69,86-88,106-107 |
Clean, well-integrated. Tests confirm path and param routes both work |
| ✅ | Performance | Phase 3: Dual Context elimination — ctx.state.set("__ctx", ctx) in rune-app.ts:120 allows route handlers to reuse the same Context |
rune-app.ts:120, module-loader.ts:162-163 |
Good — avoids creating a second Context per request |
| ✅ | Performance | Phase 5: Object.values cache on Context — paramsArray and queryValues lazy getters with private cache fields |
context.ts:22-24,83-103 |
Correct lazy caching pattern |
| ✅ | Quality | timingSafeEqual extracted to shared helper — DRY improvement across 3 middleware files |
helpers.ts, basic-auth.ts, bearer-auth.ts |
Good refactoring |
| ✅ | Quality | trailing-slash.ts simplified — Was ~72 lines, now ~15 lines via createUrlParts / isGetOrHead helpers |
trailing-slash.ts |
Clean functional decomposition |
| ✅ | Testing | 20 new tests covering fast path (4), guards (3), interceptors, static cache (2), serialization (11), context cache | Multiple test files | Good coverage. Consider adding nested-object edge cases for serializer |
| 🏗️ | Design | createLazySerializer correctly caches per shape — Compiles only once per first-encountered object shape, then reuses |
json-serializer.ts:17-30 |
This is the right pattern. The fastStringify exported function should use this same approach |
Final Verdict
fastStringifyis counter-productive — re-compiles vianew Function()on every call with no caching, making it slower thanJSON.stringify. Also a dead export. Either cache per-shape or remove.- Fast path singleton risk —
isSimplecontrollers are pre-instantiated once; document that they must be stateless, else concurrent requests share mutable state. compose()called per request — Afterseal(), the composed dispatch with the no-middleware shortcut is already pre-built. Butrune-app.ts:124still callsthis.pipeline.compose(handler)which allocates a new wrapper closure on everyfetch(). Cache the composition.
The OpenAPI "headers" → "header" fix is already correct in the diff — the previous review's concern about it was stale. The dead-code exports (compileObjectSerializer, isSerializableObject) are minor — safe to clean up but not blocking.
- Cache compiled serializers per shape in fastStringify (Map keyed by joined keys) - Use injected container c.resolve() instead of this.container in useFactory - Guard Express/Koa body serialization against undefined body - Remove silent extractor error catch; let errors propagate as 500s - Document fast path singleton stateless requirement
Owner
Author
|
Now I have a complete picture of all changed files. Here is my code review: Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Bug Risk | Fast path singleton shared across requests — isSimple controllers are new (controller as any)() at init time. Any instance property mutation (e.g., this.count++) becomes a race condition |
module-loader.ts:142 |
Document that simple controllers must be stateless; consider a factory function or Object.freeze on the instance |
|
| Security | new Function() code generation — compileObjectSerializer constructs function bodies from JSON.stringify(keys). While keys come from Object.keys(obj) (not raw user input), this bypasses CSP and is flagged by CodeQL |
json-serializer.ts:8-10 |
Add input validation that keys are safe strings; alternatively use a Map-based string builder without eval/new Function |
|
| Dead Code | 3 exports never imported — compileObjectSerializer, isSerializableObject, fastStringify are exported but never imported elsewhere. fallow-ignore-next-line pragmas confirm these are known dead exports |
json-serializer.ts:4,33,42 |
Remove dead exports or mark internal with @internal JSDoc. fastStringify has its own cache but duplicates createLazySerializer's concern |
|
| Edge Case | Fast path createLazySerializer caches per-first-object-shape — If the first response is an array (falls back to JSON.stringify), all subsequent serializations use JSON.stringify even for plain objects |
json-serializer.ts:20-27 |
Edge case is unlikely (first response typically sets the pattern), but worth noting. Consider resetting compiled on type mismatch | |
| Maintainability | moduleGuards heuristic is fragile — Filters providers by p.name.endsWith("Guard") or "canActivate" in p.prototype. Arrow-function class properties lack prototype; naming convention is brittle |
module-loader.ts:64-66 |
Use explicit metadata from @UseGuard at module level instead of convention-based heuristics |
|
| 🔧 | Minor | Fast path extractors accept but ignore _params — case "body", case "query", and others declare _params: Record<string, string> parameter but never use it |
module-loader.ts:148-161 |
Remove unused parameter to reduce closure size |
| 🔧 | Minor | useFactory parameter c shadows injected container — Line 77 passes (c: IContainer) but the surrounding scope also has this.container. The code correctly uses c in the factory, but _c would clarify intent |
module-loader.ts:77 |
Rename to _c or just verify the correct container is used (it is) |
| 🔧 | Minor | Compose closure created per request — this.pipeline.compose(handler) at rune-app.ts:124 creates a new wrapper closure on every fetch() call, even in the sealed no-middleware fast path |
rune-app.ts:124 |
In the seal() fast path, the composed wrapper is minimal (1 async call + 1 instanceof check). Acceptable but worth noting for future optimization |
| 🔧 | Minor | ctx.state mutation via Map — ctx.state.set("__ctx", ctx) at rune-app.ts:120 stores the context on itself. The handler middleware receives ctx as its parameter, so __ctx retrieval in module-loader.ts:165 is always the same reference |
rune-app.ts:120, module-loader.ts:165 |
This is functionally correct but the round-trip is unnecessary — the ctx is already available as the handler's first argument. Not a bug, just an indirect pattern |
| ✅ | Performance | Phase 7: Router static cache — Routes without : or * get O(1) Map lookup bypassing radix tree |
router.ts:69,86-88,106-107 |
Clean integration; tests cover both static and param routes |
| ✅ | Performance | Phase 5: Object.values cache on Context — paramsArray and queryValues lazy getters with private cache fields |
context.ts:22-24,83-103 |
Correct lazy caching pattern |
| ✅ | Performance | Phase 4: Middleware pipeline seal — seal() pre-builds this.composed to avoid conditionals on hot path |
middleware-pipeline.ts:39-62 |
Clean design; the fallback compose() path at lines 64-91 is only reachable if seal() wasn't called |
| ✅ | Bug Fix | OpenAPI "headers" → "header" — Corrects OpenAPI 3.0 spec violation (spec requires singular "header") |
scanner.ts:134 |
Fixed correctly; test expects "header" at scanner.test.ts:119 |
| ✅ | Bug Fix | Adapter body handling — Handles pre-parsed body objects vs raw strings in Express, Koa, Fastify adapters | express/src/index.ts:63-68, koa/src/index.ts:29-35, fastify/src/index.ts:56-61 |
Correct defensive fix with typeof + != null check |
| ✅ | Quality | timingSafeEqual extracted to shared helper — DRY improvement across basic-auth, bearer-auth, trailing-slash |
helpers.ts, basic-auth.ts, bearer-auth.ts |
Clean refactoring |
| ✅ | Quality | trailing-slash.ts simplified — ~72 lines → 71 lines via createUrlParts / isGetOrHead helpers |
trailing-slash.ts |
Good functional decomposition |
| ✅ | Quality | Container.createScope takes parent reference — initChild now receives parent for clearer chain management |
container.ts:31-39 |
Clean improvement |
| ✅ | Testing | 20 new tests across 4 test files — Fast path (4), guards (3), interceptors, static cache (2), serialization (11), context cache | rune-app.test.ts:725-840, json-serializer.test.ts, router.test.ts:108-122, context.test.ts |
Good coverage — consider adding nested-object edge case for serializer, and a test verifying fastStringify shape cache distinctness |
Final Verdict
new Function() CSP bypass in compileObjectSerializer (document / mitigate), the 3 dead exports with fallow-ignore pragmas (fastStringify should either be used or removed), and the fast path shared-instance risk (documented but no enforcement). The moduleGuards heuristic and compose per-request allocation are minor design notes. Overall, the code quality is high and the performance gains are meaningful.
- Remove export from compileObjectSerializer and isSerializableObject (internal) - Add SAFE_KEY_RE validation to compileObjectSerializer (falls back to JSON.stringify for non-safe keys) - Remove compileObjectSerializer direct tests (covered by createLazySerializer/fastStringify) - Add Object.freeze to fast path singleton to enforce statelessness at runtime
Owner
Author
|
Now I have a thorough understanding of all changes. Here is my review:
Final Verdict: 🔴 Changes requested. The optimizations are well-designed and the code quality is generally high, but there is one correctness bug that must be fixed: |
…router cache - Inline handler directly in fetch() when pipeline has no middleware (eliminates 2 closure allocations per request) - Skip Promise.all when all param extractors are synchronous (most common case: param/query/headers only) - Use two-level Map (method -> pathname) in router static cache (avoids per-request template literal string allocation) - Share JSON headers constant across modules (avoids per-response object allocation)
Owner
Author
Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | createLazySerializer doesn't recompile on shape mismatch — Compiles keys from first response object, then reuses forever. If a route returns {id:1} then later {name:"x"}, the output becomes {"id":undefined} — invalid JSON with literal undefined |
json-serializer.ts:23-31 |
Compare current object keys against compiled keys; fall back to JSON.stringify on mismatch |
| Performance | fastStringify dead export with fallow-ignore — Exported but never imported. Uses per-shape caching but duplicates createLazySerializer's concern. Adds 200B+ to bundle |
json-serializer.ts:45 |
Remove dead export; the fast path already uses createLazySerializer internally |
|
| Security | new Function() code generation — compileObjectSerializer constructs function body from JSON.stringify(keys). While keys come from Object.keys() (not user input), this bypasses CSP and is flagged by CodeQL |
json-serializer.ts:12-15 |
Document CSP restriction, or use a Map-based string builder without eval/new Function |
|
| Bug Risk | Fast path singleton mutable via nested references — Object.freeze is shallow: this.cache.set(k,v) or this.items.push(x) still mutates shared state across concurrent requests |
module-loader.ts:145 |
Document strongly that simple controllers must be stateless (including nested collections). Consider deepFreeze or a factory |
|
| Bug Risk | moduleGuards heuristic is fragile — Filters by p.name.endsWith("Guard") or "canActivate" in p.prototype. Misses arrow-function guards (no prototype), false-positives on classes coincidentally named *Guard |
module-loader.ts:66-67 |
Use explicit metadata from @UseGuard at module-level instead of convention-based heuristic |
|
| Maintainability | Error handling asymmetry — fast path vs normal path — Fast path (line 188) lets extraction errors propagate as 500. Normal executeHandler (line 262) silently swallows errors and sets args = []. Same intent, different failure modes |
module-loader.ts:188,262 |
Normalize: either both propagate or both fall back. Document the behavior | |
| Performance | compose() allocates closure per request — this.pipeline.compose(handler) in rune-app.ts:145 creates a new wrapper function on every fetch(), even when sealed with no middleware |
middleware-pipeline.ts:45-50, rune-app.ts:145` |
Pre-assign the no-middleware wrapper to avoid per-request compose call, or inline the handler directly like the sealed fast path already does at line 125 |
|
| 🔧 | Minor | __ctx round-trip indirection — Handler receives context.state (Map) as 3rd arg, then does _context.get("__ctx") to get the Context back. The Context is already available as the first arg to the middleware |
module-loader.ts:184,203, rune-app.ts:120-122 |
Pass context directly (via state.set) is only needed because RouteHandler signature is Map-typed. Consider changing handler signature — or keep as is since it's a minor indirection |
| 🔧 | Minor | Fast path extractors declare unused _params parameter — Several extractors like "body", "headers", "context" cases declare _params but never read it |
module-loader.ts:153,159,164,170,176 |
Remove unused parameter from those extractors |
| 🔧 | Minor | Adapter body: req.body could be undefined or Buffer — typeof undefined === "undefined" falls through to req.body != null (false → undefined). But Buffer bodies pass typeof check as "object" and JSON.stringify(buffer) produces {} — data loss |
express/src/index.ts:63-68, koa/src/index.ts:29-35, fastify/src/index.ts:56-61 |
Add Buffer.isBuffer(req.body) check, or handle undefined explicitly before the typeof branch |
| 🔧 | Minor | compose() fallback path (not sealed) still creates closures — The unsealed compose path at line 70-94 creates a new dispatch closure each call. In practice, seal() is always called before first fetch() |
middleware-pipeline.ts:68-95 |
Consider making unsealed compose throw, or document that seal() must be called |
| 💡 | Observation | Non-fast path still uses JSON.stringify — executeHandler line 274 uses JSON.stringify(result) instead of createLazySerializer |
module-loader.ts:274 |
Could use the lazy serializer for consistency with the fast path |
| ✅ | Performance | Phase 7: Router static cache — O(1) Map lookup for !path.includes(":") && !path.includes("*"), clean integration |
router.ts:69,86-93,111-115 |
Well-implemented |
| ✅ | Performance | Phase 5: paramsArray/queryValues lazy caching — Private cache fields + lazy getters; test verifies reference identity |
context.ts:22-24,83-103 |
Correct pattern |
| ✅ | Performance | Phase 4: Middleware pipeline seal — Pre-builds composed dispatch, sealedNoMiddleware getter bypasses chain |
middleware-pipeline.ts:35-66 |
Clean, efficient |
| ✅ | Quality | timingSafeEqual extracted to helpers.ts — DRY across basic-auth, bearer-auth, trailing-slash |
helpers.ts, basic-auth.ts, bearer-auth.ts |
Good refactoring |
| ✅ | Quality | trailing-slash.ts refactored with createUrlParts/isGetOrHead — Clean functional decomposition |
trailing-slash.ts |
Much cleaner |
| ✅ | Quality | Container initChild takes explicit parent param — Clearer chain management |
container.ts:31-39 |
Good improvement |
| ✅ | Fix | OpenAPI "headers" → "header" — Correct OpenAPI 3.0 spec compliance |
scanner.ts:129-136 |
Correctly fixed; test confirms |
| ✅ | Fix | Adapter body handling — Pre-parsed body objects vs raw strings, correct defensive pattern | express, koa, fastify adapters |
Correct |
| ✅ | Testing | 20 new tests — Fast path (4), guards (3), serializer (11), cache (2), context caching (2) | Multiple test files | Good coverage. Consider adding: serializer shape-mismatch test, fast-path concurrent state mutation test, Buffer body edge case |
Final Verdict
Required before merging:
- 🔴
createLazySerializershape-mismatch bug — produces invalid JSON when response keys change between requests ⚠️ Remove deadfastStringifyexport (200B+ bundle savings, confirmed dead by fallow)⚠️ Normalize error handling between fast path (500) and normal path (silentargs = [])
Strongly recommended:
4. compose() closure allocation — cache the wrapper
5.
6. moduleGuards heuristic fragility
The remaining items (CSP concern, adapter Buffer edge case, unused params) are minor.
…ndling, heuristic
- Fix createLazySerializer shape-mismatch: recompile serializer when
object keys change between calls (tracks shapeKey via join("\0"))
- Remove dead fastStringify export (confirmed unused, saves bundle size)
- Normalize error handling: remove silent try/catch in executeHandler
(let param extraction errors propagate like the fast path now does)
- Document nested mutability caveat in fast path comment
- Fix moduleGuards heuristic: remove fragile p.name.endsWith("Guard")
check, rely solely on canActivate in prototype
Owner
Author
|
Now I have a complete view of all changes. Let me produce the review. Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Bug | createLazySerializer produces invalid JSON on type switch — When compiled for objects ({msg:"hello"}), then called with an array/primitive, the old compiled function runs against the wrong type producing {"msg":undefined} (literal undefined in JSON). E.g., serialize([1,2,3]) after serialize({msg:"x"}) yields {"msg":undefined} |
json-serializer.ts:23-37 |
Reset compiled = null in the else branch so arrays/primitives fall through to JSON.stringify, and subsequent objects recompile |
|
| Breaking Change | Container.useClass removed — Throws "useClass is not supported". All existing useClass registrations must be migrated to useFactory. This was not mentioned in the PR description or docs |
container.ts:103-108 |
Document in AGENTS.md or the PR body. The test suite migration shows the pattern (c.register({ token: Service, useFactory: () => new Service() })) |
|
| 🔧 | Maintainability | _context?.get?.("__ctx") unnecessary optional chaining — _context is typed as Map<string, unknown> (per RouteHandler signature), always defined. The ?. operators are misleading and suggest _context could be nullish |
module-loader.ts:165,183 |
Use _context.get("__ctx") directly — the Map is always present |
| 💡 | Minor | hasSyncExtractors check short-circuits error handling — When hasSyncExtractors is true, a throwing extractor (e.g., ctx.body on a non-body request) propagates synchronously as an uncaught exception instead of a rejected promise |
module-loader.ts:171-173 |
Wrap the map in a try block or use Promise.all consistently regardless of sync/async — currently the sync path will crash rather than produce a 500 |
| ✅ | Bug Fix | createLazySerializer recompiles on shape change — Latest commit added shapeKey tracking, correctly re-compiling when object keys differ between requests |
json-serializer.ts:27-29 |
Fix verified — test confirms round-trip through different shapes |
| ✅ | Bug Fix | Dead exports removed — fastStringify deleted, compileObjectSerializer and isSerializableObject made non-exported functions |
json-serializer.ts |
Clean |
| ✅ | Bug Fix | Error handling unified — Both fast path and normal path now propagate extraction errors as 500s (try/catch removed from executeHandler) |
module-loader.ts |
Consistent behavior |
| ✅ | Bug Fix | moduleGuards heuristic simplified — Changed from `p.name.endsWith("Guard") |
"canActivate" in p.prototypeto just"canActivate" in p.prototype` |
|
| ✅ | Fix | OpenAPI "headers" → "header" — Correct OpenAPI 3.0 spec compliance |
scanner.ts:134 |
Correct; test expects "header" |
| ✅ | Fix | Adapter body handling — Handles pre-parsed body objects vs raw strings across Express, Koa, Fastify | express/koa/fastify adapters |
Correct defensive pattern |
| ✅ | Performance | Router static cache (Phase 7) — O(1) Map lookup for routes without : or * |
router.ts:69,86-93,111-115 |
Clean, well-integrated |
| ✅ | Performance | Middleware pipeline seal (Phase 4) — seal() pre-builds composed dispatch; sealedNoMiddleware getter bypasses chain allocation in fetch() |
middleware-pipeline.ts:35-66, rune-app.ts:125-146 |
Inline no-middleware path avoids per-request compose() call |
| ✅ | Performance | paramsArray/queryValues lazy caching (Phase 5) — Private paramsArrayCache/queryValuesCache fields with lazy getters on Context |
context.ts:83-103 |
Tests verify reference identity |
| ✅ | Performance | Dual Context elimination (Phase 3) — ctx.state.set("__ctx", ctx) so handlers reuse the same Context instance |
rune-app.ts:132, module-loader.ts:165,183 |
Correct |
| ✅ | Performance | Fast path for simple controllers (Phase 1) — Pre-instantiates controllers without deps/guards/interceptors/DTOs at init time, skipping DI resolution per request | module-loader.ts:132-178 |
Correct; Object.freeze with statelessness comment |
| ✅ | Performance | Pre-compiled JSON serializer (Phase 2) — createLazySerializer compiles optimized serializers per response shape |
json-serializer.ts |
Correct pattern with lazy compilation |
| ✅ | Quality | timingSafeEqual extracted to shared helpers.ts — DRY across basic-auth.ts, bearer-auth.ts, trailing-slash.ts |
helpers.ts |
Good refactoring |
| ✅ | Quality | trailing-slash.ts decomposed — createUrlParts/isGetOrHead helpers reduce duplication |
trailing-slash.ts |
Clean |
| ✅ | Quality | joinPaths extracted to @rune/decorators — Shared between ModuleLoader and OpenAPIScanner |
metadata.ts:138-145 |
Good DRY improvement |
| ✅ | Quality | Container initChild uses proper assignments — Replaced (this as any).parent = parent with this.parent = parent |
container.ts:38-40 |
Type-safe |
| ✅ | Quality | isSerializableObject handles undefined — Checks val === undefined in addition to null |
json-serializer.ts:40 |
Correct |
| ✅ | Testing | 20 new tests across 5 test files — Fast path (4), guards (1), serializer (4), static cache (2), context caching (2), container migration (7) | Various | Good coverage |
| 📝 | Docs | Benchmark title still says "Phase 1-7 optimizations to beat Fastify" — The commit message a3755f3 and benchmark docs still reference "beating Fastify" |
Commit message, docs/benchmarks.md |
Update to "toward Fastify parity" — actual benchmarks show Rune at ~62% of Fastify throughput |
Final Verdict
✅🎯 LGTM (minor). The 14-commit optimization PR delivers genuine throughput improvements across all phases. The three previous rounds of review feedback have been addressed in the latest commits (shape-mismatch fix, dead export removal, error handling unification, heuristic cleanup). The remaining issues are a minor edge case in createLazySerializer (type-switch produces invalid JSON — unlikely in practice since routes return consistent types) and an undocumented breaking change (useClass removed from Container). Both are worth fixing but not blocking. The optional chaining on _context.get() is cosmetic.
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
2 participants
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.
Summary
Performance optimization implementing multiple phases to improve Rune's HTTP throughput.
Changes
new Function(), replacingJSON.stringify()on the hot path.fetch()stores Context onctx.state('__ctx')so route handlers reuse it instead of creating a second one.compose()short-circuits when no middleware is registered.Object.valuescache on Context —paramsArrayandqueryValueslazy getters cache results.:or*patterns cached in aMapfor O(1) hash-based lookup.Testing