perf+types: hoist per-request invariants and tighten lib/index.d.ts#273
Open
arb wants to merge 3 commits into
Open
perf+types: hoist per-request invariants and tighten lib/index.d.ts#273arb wants to merge 3 commits into
arb wants to merge 3 commits into
Conversation
Move work that's fully determined at celebrate() call time out of the per-request middleware closure: - Look up validateRequest (mode -> impl) once at construction. - Merge the Joi.compile pass with the canonical-segment-order walk into a single iteration over internals.REQ_VALIDATIONS, dropping the intermediate Map. - When reqContext is falsy (the default and common case), precompute joiConfig and the bound steps once. Per request, reuse the cached steps array directly. When reqContext is true, fall back to building steps per request because joiConfig.context must reference req. Behavior is unchanged: same validation order, same error shapes, same req-mutation semantics, same middleware._schema exposure, same return-promise-for-tests contract. All 62 tests pass with 100% coverage retained. utils/benchmark.js comparison (avg of 3 runs each, throughput ops/s): valid 1,215,325 -> 1,378,617 (+13.4%) invalid partial 238,711 -> 245,241 (+2.7%) invalid full 103,842 -> 106,556 (+2.6%) Latency averages move in lockstep: valid 816.7ns -> 743.6ns (-8.9%, steady-state) invalid partial 4,308.1ns -> 4,201.6ns (-2.5%) invalid full 9,872.9ns -> 9,639.3ns (-2.4%) The "valid" path benefits most because the hoist eliminates the dominant non-Joi setup cost on a successful request. The invalid paths gain less because Joi's rejection path (building ValidationError, walking details) dominates total time, so the saved setup overhead is a smaller share. Co-authored-by: Cursor <cursoragent@cursor.com>
Five small fixes to the hand-written declaration file. No runtime code
is touched. Verified with tsc --noEmit against a throwaway consumer
smoke-test plus the existing test suite (62/62 passing, 100% coverage
retained) and the existing eslint pass.
- errors() opts type now accepts { statusCode?: number; message?: string }
to match the runtime schema (lib/celebrate.js, ERRORSOPTSSCHEMA in
lib/schema.js). Previously statusCode was wrongly required and message
was missing entirely. Externally visible.
- errors() generics removed. The <P, ResBody, ReqBody, ReqQuery>
parameters were forwarded to ErrorRequestHandler but never narrowed
anything because error middleware doesn't differentiate request types.
Externally visible to callers writing errors<MyParams>(...).
- Modes and Segments switched from `declare enum` to `declare const` +
type alias. This matches the runtime shape (plain const objects in
lib/constants.js) and gives consumers literal-string types instead of
nominal enum-member types. Comparisons like `seg === 'params'` that
previously hit "no overlap" errors under strict configs now type-check
cleanly. Externally visible.
- SchemaOptions segments retyped from `object` to `Joi.SchemaLike`.
Joi 18 ships SchemaLike for exactly this case. Runtime Joi.compile
already accepts both compiled Joi.Schema and plain object schemas,
so no behavior change.
- Celebrator2 made generic with the same defaults as Celebrator1 and
Celebrator, so generics propagate through the full
celebrator(opts)(joiOpts)(schema) curry chain. Previously the third
call returned a non-generic RequestHandler and dropped any narrowing
the consumer had set up.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #273 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 263 265 +2
=========================================
+ Hits 263 265 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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.
Two-commit branch out of an audit of
lib/*for non-idiomatic JS, perf, and type-definition tightness. Both commits are independently revertable; the perf one is the only behavior change.What's in here
1.
perf: hoist per-request invariants out of celebrate() middleware(c3dba2a)Most of what
celebrate()was doing per request is fully determined the moment the middleware is constructed. This commit moves that work out of the closure:mode-> impl lookup (internals.validateFns[finalOpts.mode]) is now resolved once at construction.Object.entries(_requestRules)to compile and theninternals.REQ_VALIDATIONSto filter are merged into a single iteration over the canonical segment order, dropping the intermediateMap.reqContextis falsy (the default and common case),joiConfigand the boundstepsarray are precomputed once. Per request the cachedstepsis reused directly. WhenreqContextis true, we still rebuildstepsper request becausejoiConfig.contextmust referencereq.Behavior is unchanged: same canonical validation order, same
CelebrateError.detailsshape, sameObject.definePropertysemantics onreq, samemiddleware._schemaexposure, same return-promise-for-tests contract. All 62 tests still pass with 100% coverage retained.utils/benchmark.jsdeltas (avg of 3 runs each, throughput ops/s):valid(POST, body-only schema, partial mode)invalid partial(POST, body-only schema, fails)invalid full(POST, body+query+params, full mode, all fail)The
validpath benefits most because the hoist removes the dominant non-Joi setup cost on a successful request. The invalid paths gain less because Joi's rejection path (buildingValidationError, walking details) dominates total time, so the saved setup overhead is a smaller share.reqContext: truecallers see no regression — that path is unchanged.2.
types: tighten lib/index.d.ts to match runtime(7bd8e16)Five small fixes to the hand-written declaration file. No runtime code is touched.
errors()opts shape — was{ statusCode: number }(statusCode wrongly required,messagemissing). Now{ statusCode?: number; message?: string }, matchingERRORSOPTSSCHEMAinlib/schema.jsand the runtime defaulting inlib/celebrate.js.errors()generics removed —<P, ResBody, ReqBody, ReqQuery>were forwarded toErrorRequestHandlerbut never narrowed anything because error middleware doesn't differentiate request types.Modes/Segments— switched fromdeclare enumtodeclare const+ type alias. Matches the runtime shape (plain const objects inlib/constants.js), gives consumers literal-string types instead of nominal enum-member types, and lets comparisons likeseg === 'params'type-check cleanly under strict configs.SchemaOptionssegments — retyped fromobjecttoJoi.SchemaLike. Joi 18 shipsSchemaLikefor exactly this case andJoi.compilealready accepts both compiledJoi.Schemaand plain object schemas at runtime.Celebrator2— made generic with the same defaults asCelebrator1andCelebrator, so generics propagate through the fullcelebrator(opts)(joiOpts)(schema)curry chain. Previously the third call returned a non-genericRequestHandlerand dropped any narrowing the consumer had set up.Externally visible to consumers in three places: anyone calling
errors<MyParams>(...)will need to drop the type args; anyone usingSegments/Modesenum-member types in type position (e.g.function f(s: Segments.PARAMS)) gets the literal string instead; anyone passing a non-SchemaLikevalue intoSchemaOptionssegments will now see a type error. No runtime impact in any case.Verification
CI=true npm test— 62/62 passing, 100% coverage retained on everylib/*.jsfile.npx eslint .— clean.node_modules/.bin/tsc --noEmit -p tmp/agent/tsconfig.json— clean against a throwaway consumer smoke-test exercising every public export, including@ts-expect-errorlines for negative cases (kept intmp/agent/, not committed).node_modules/.bin/tsc --noEmit ... lib/index.d.tsstandalone — clean.utils/benchmark.jsperf comparison documented above.Matrix safety
CI matrix in
.github/workflows/ci.ymlis Node20.x, 22.x, 24.x, 25.x× Expresslatest-4, latest. Both commits work across the full matrix:Map,Array.prototype.map, object spread, async/await). The.then(next).catch(next)is preserved because Express 4 doesn't auto-forward async-middleware rejections (Express 5 does, but we support both)..d.tsdeclarations, zero runtime effect.QA Spec
lib/index.d.tsconsumers in your downstream apps (if any TS) still type-check; fix-ups limited to the three externally-visible items abovereqContext: trueflows (covered by existing tests; benchmark only exercisesreqContext: false)errors()default behavior unchanged: 400 statusCode, joi error message, validation map keyed by segmentcelebratorcurried call shapes — all four — still type-check at consumer call sitesFuture cleanup spotted in the audit (not in this PR)
Object.defineProperty(req, segment, { value })inlib/celebrate.jsseals the validated segment as non-writable / non-configurable / non-enumerable. Documented behavior is "overwrite", not "seal" — chainedcelebrate()calls on the same segment throwTypeError: Cannot redefine property. Worth either documenting or loosening the descriptor.lodash.curry+lodash.flipcould be replaced with ~8 lines of native code on Node 20+.EscapeHtmlon JSON response keys is unusual for application/json APIs; defense-in-depth, but worth a deliberate decision.Made with Cursor