feat(server): implement automatic error map merging for middleware #924
Conversation
- Add errorMap property to DecoratedMiddleware interface - Automatically merge middleware error maps when using .use() method - Update Builder and DecoratedProcedure to handle error map merging - Add comprehensive tests for error middleware patterns - Maintain backward compatibility with existing middleware usage This change ensures that errors thrown from middleware are properly recognized as "defined" errors, fixing the error handling issue where middleware errors were not being properly typed or handled.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
bot
added
the
size:L
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds error-map propagation to decorated middlewares: decorated instances can carry an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as Builder
participant Merge as mergeErrorMap
participant Decorate as decorateMiddleware
Dev->>Builder: use(middleware)
alt middleware.errorMap present
Builder->>Merge: merge(builder.errorMap, middleware.errorMap)
Merge-->>Builder: mergedErrorMap
Builder->>Decorate: decorateMiddleware(middleware, mergedErrorMap)
Decorate-->>Builder: DecoratedMiddleware{errorMap: mergedErrorMap}
Builder-->>Dev: New Builder (with merged errorMap)
else no middleware.errorMap
Builder->>Decorate: decorateMiddleware(middleware, builder.errorMap)
Decorate-->>Builder: DecoratedMiddleware
Builder-->>Dev: New Builder (errorMap unchanged/omitted)
end
sequenceDiagram
autonumber
actor Dev as Developer
participant Proc as DecoratedProcedure
participant Merge as mergeErrorMap
participant Decorate as decorateMiddleware
Dev->>Proc: use(middleware)
alt middleware.errorMap present
Proc->>Merge: merge(proc.errorMap, middleware.errorMap)
Merge-->>Proc: mergedErrorMap
Proc->>Decorate: decorateMiddleware(middleware, mergedErrorMap)
Decorate-->>Proc: DecoratedMiddleware{errorMap: mergedErrorMap}
Proc-->>Dev: DecoratedProcedure (with merged errorMap)
else no middleware.errorMap
Proc->>Decorate: decorateMiddleware(middleware, proc.errorMap)
Decorate-->>Proc: DecoratedMiddleware
Proc-->>Dev: DecoratedProcedure (middlewares updated, no errorMap merge)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @QzCurious, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces automatic merging of error maps from middleware into the main application or procedure's error map. Previously, errors thrown from middleware might not have been correctly identified as "defined" errors within the ORPC framework. This change enhances the error handling mechanism by ensuring that all errors defined within middleware are properly recognized and typed, improving consistency and debugging for developers.
Highlights
- Automatic Error Map Merging: Implemented a mechanism to automatically merge error maps defined within middleware when they are used with Builder or DecoratedProcedure's .use() method. This ensures that errors originating from middleware are correctly typed and recognized as "defined" errors.
- Enhanced DecoratedMiddleware: The DecoratedMiddleware interface now includes an errorMap property, allowing middleware to carry its own set of defined errors.
- Updated Middleware Decoration and Concatenation: The decorateMiddleware function and the concat method for decorated middleware now correctly propagate and merge error maps, maintaining the integrity of error definitions across chained middleware.
- Comprehensive Error Handling Tests: A new test file, error-middleware-pattern.test.ts, has been added to validate the new error merging behavior and ensure proper error recognition in various middleware usage scenarios.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully implements automatic error map merging for middleware, which is a great enhancement for error handling. The changes are well-structured, and the addition of comprehensive tests in error-middleware-pattern.test.ts is excellent for ensuring correctness. I have a few suggestions to improve maintainability by refactoring some duplicated code and a high-severity suggestion to fix a potential bug where an existing errorMap on a middleware could be lost.
| // Attach error map if provided | ||
| if (errorMap) { | ||
| decorated.errorMap = errorMap | ||
| } |
There was a problem hiding this comment.
The decorateMiddleware function doesn't preserve the errorMap if the passed middleware is already a DecoratedMiddleware and no new errorMap is provided. This can lead to the errorMap being lost when chaining methods like .mapInput(). To fix this, you should check for an existing errorMap on the middleware and preserve it.
// Attach error map if provided, preserving existing one if present
if (errorMap) {
decorated.errorMap = errorMap
} else if ('errorMap' in middleware && (middleware as AnyDecoratedMiddleware).errorMap) {
decorated.errorMap = (middleware as AnyDecoratedMiddleware).errorMap
}There was a problem hiding this comment.
What about this? I want it typed.
export function decorateMiddleware<
TInContext extends Context,
TOutContext extends Context,
TInput,
TOutput,
TErrorConstructorMap extends ORPCErrorConstructorMap<any>,
TMeta extends Meta,
>(
middleware: Middleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>
| DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>,
errorMap?: ErrorMap,
): DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> {
const decorated = ((...args) => middleware(...args)) as DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>
// Attach error map if provided
if (errorMap) {
decorated.errorMap = errorMap
}
if ('errorMap' in middleware) {
decorated.errorMap = middleware.errorMap
}| if (!('errorMap' in middleware) || !middleware.errorMap) { | ||
| return new Builder({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| }) as any | ||
| } | ||
|
|
||
| return new Builder({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| errorMap: mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap), | ||
| }) as any |
There was a problem hiding this comment.
The logic inside the use method has some code duplication between the if and the else path. You can refactor it to be more concise and maintainable by creating the new builder definition object and conditionally adding the errorMap.
const errorMap = ('errorMap' in middleware && middleware.errorMap)
? mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap)
: this['~orpc'].errorMap
return new Builder({
...this['~orpc'],
middlewares: addMiddleware(this['~orpc'].middlewares, mapped),
errorMap,
}) as anyThere was a problem hiding this comment.
It's about whether the "errorMap" exist or not. The given solution is conciser but would make the object {errorMap: undefined} instead of omit the "errorMap" property. I don't know if it would affect anytime so I better make it safer.
| if (!('errorMap' in middleware) || !middleware.errorMap) { | ||
| return new DecoratedProcedure({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| }) | ||
| } | ||
|
|
||
| return new DecoratedProcedure({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| errorMap: mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap), | ||
| }) |
There was a problem hiding this comment.
Similar to the use method in builder.ts, this implementation has duplicated code. It can be refactored for better readability and maintainability.
const errorMap = ('errorMap' in middleware && middleware.errorMap)
? mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap)
: this['~orpc'].errorMap
return new DecoratedProcedure({
...this['~orpc'],
middlewares: addMiddleware(this['~orpc'].middlewares, mapped),
errorMap,
})
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/server/src/middleware-decorated.ts (3)
101-105: Make internal errorMap non-enumerable to avoid leaking internal detailsSince errorMap is marked @internal and attached to a function object, setting it as a non-enumerable property prevents accidental exposure during object spreads or logging.
Apply:
- // Attach error map if provided - if (errorMap) { - decorated.errorMap = errorMap - } + // Attach error map if provided (keep internal) + if (errorMap) { + Object.defineProperty(decorated, 'errorMap', { + value: errorMap, + enumerable: false, + configurable: true, + writable: true, + }) + }
115-118: Preserve concat-middleware’s errorMap on the inner mapped wrapper as wellWhen mapInput is provided, decorateMiddleware(concatMiddleware) drops any existing errorMap for the inner mapped wrapper. You already pass the combined map to the outer decoration, so behavior is correct; however, preserving the inner wrapper’s errorMap improves debuggability and symmetry with mapInput above.
- const mapped = mapInput - ? decorateMiddleware(concatMiddleware).mapInput(mapInput) + const mapped = mapInput + ? decorateMiddleware( + concatMiddleware, + 'errorMap' in concatMiddleware ? concatMiddleware.errorMap : undefined, + ).mapInput(mapInput) : concatMiddleware
120-124: Use contract’s mergeErrorMap helper for consistencyYou’re manually spreading error maps here while other places use mergeErrorMap. Prefer the helper to centralize semantics (in case merge rules evolve).
- const combinedErrorMap = { - ...decorated.errorMap, - ...('errorMap' in concatMiddleware ? concatMiddleware.errorMap : undefined), - } + const combinedErrorMap = mergeErrorMap( + (decorated as AnyDecoratedMiddleware).errorMap ?? {}, + ('errorMap' in concatMiddleware ? concatMiddleware.errorMap : undefined) ?? {}, + )Note: import mergeErrorMap from @orpc/contract at the top of this file.
packages/server/src/procedure-decorated.ts (2)
140-144: Optionally preserve middleware.errorMap on the mapped wrapperSimilar to middleware-decorated.concat, when mapInput is provided you re-wrap the middleware without carrying its errorMap. Functionally fine since you merge into the procedure’s errorMap below; preserving it on the wrapper can aid debugging.
- const mapped = mapInput - ? decorateMiddleware(middleware).mapInput(mapInput) + const mapped = mapInput + ? decorateMiddleware( + middleware as AnyDecoratedMiddleware, + 'errorMap' in middleware ? middleware.errorMap : undefined, + ).mapInput(mapInput) : middleware
152-156: Merging procedure errorMap with middleware.errorMap is correct and order-safemergeErrorMap(existing, incoming) gives precedence to the middleware’s definitions on key conflicts, which matches expectations. Consider adding a regression test where keys collide to document this precedence.
I can add a focused test to assert collision precedence if desired.
packages/server/tests/error-middleware-pattern.test.ts (2)
48-79: Add a concat-path test to exercise decorated.concat mergingCurrent tests cover builder.use and base errors. Consider adding a case where two decorated middlewares are concatenated (with and without mapInput) to ensure combined errorMap flows through concat, not just use.
Here’s a minimal pattern you can adapt:
+ it('should merge error maps through decorated.concat (with and without mapInput)', async () => { + const mwA = os.errors({ UNAUTHORIZED: {} }).middleware(async ({ next }) => next({})) + const mwB = os.errors({ NOT_FOUND: {} }).middleware(async ({ next }) => next({})) + + // concat without mapInput + const combined1 = mwA.concat(mwB) + + // concat with mapInput + const combined2 = mwA.concat(mwB, (_: any) => _) + + const r1 = os.use(combined1).handler(async ({ errors }) => { throw errors.NOT_FOUND() }) + const r2 = os.use(combined2).handler(async ({ errors }) => { throw errors.UNAUTHORIZED() }) + + const c1 = createSafeClient(createRouterClient(r1)) + const c2 = createSafeClient(createRouterClient(r2)) + + const [e1,, d1] = await c1() + const [e2,, d2] = await c2() + + expect(d1).toBe(true); expect((e1 as any).code).toBe('NOT_FOUND') + expect(d2).toBe(true); expect((e2 as any).code).toBe('UNAUTHORIZED') + })
63-66: Tracking the inference limitation is good; consider a follow-up issueThe @ts-expect-error notes the current type inference gap for middleware-provided errors. Worth opening an issue to track improving ErrorMap inference across use/concat boundaries.
I can draft a proposal for propagating middleware ErrorMap types through Builder/Procedure generics if helpful.
packages/server/src/builder.ts (2)
192-205: use: mapping path drops middleware.errorMap on the wrapper (optional)As in other sites, when mapInput is present you re-wrap the incoming middleware without carrying its errorMap. Functional behavior remains correct because you merge into the Builder’s errorMap below; preserving it on the wrapper can aid debugging and future extensions.
- const mapped = mapInput - ? decorateMiddleware(middleware).mapInput(mapInput) + const mapped = mapInput + ? decorateMiddleware( + middleware as AnyDecoratedMiddleware, + 'errorMap' in middleware ? middleware.errorMap : undefined, + ).mapInput(mapInput) : middleware
200-211: Conditional merge on presence of middleware.errorMap is correct; consider a collision testMerging only when truthy keeps backward compatibility. Add a unit test where the router and middleware define the same code to document precedence (middleware overwrites router on same key).
I can extend builder.test.ts with a focused case asserting merge precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/server/src/builder.test.ts(1 hunks)packages/server/src/builder.ts(3 hunks)packages/server/src/middleware-decorated.ts(5 hunks)packages/server/src/procedure-decorated.ts(2 hunks)packages/server/tests/error-middleware-pattern.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/server/tests/error-middleware-pattern.test.ts (4)
packages/server/src/builder.ts (4)
os(344-360)middleware(146-150)errors(158-165)router(325-329)packages/server/src/procedure-decorated.ts (1)
errors(39-53)packages/client/src/client-safe.ts (1)
createSafeClient(24-42)packages/server/src/router-client.ts (1)
createRouterClient(27-71)
packages/server/src/middleware-decorated.ts (3)
packages/contract/src/error.ts (1)
ErrorMap(31-33)packages/server/src/builder.ts (2)
middleware(146-150)input(251-259)packages/server/src/middleware.ts (2)
AnyMiddleware(63-63)MapInputMiddleware(65-67)
packages/server/src/procedure-decorated.ts (5)
packages/server/src/builder.ts (1)
middleware(146-150)packages/server/src/middleware.ts (2)
AnyMiddleware(63-63)MapInputMiddleware(65-67)packages/server/src/middleware-decorated.ts (2)
AnyDecoratedMiddleware(86-86)decorateMiddleware(88-142)packages/server/src/middleware-utils.ts (1)
addMiddleware(29-31)packages/contract/src/error.ts (1)
mergeErrorMap(37-39)
packages/server/src/builder.ts (4)
packages/server/src/middleware-decorated.ts (2)
decorateMiddleware(88-142)AnyDecoratedMiddleware(86-86)packages/server/src/middleware.ts (2)
AnyMiddleware(63-63)MapInputMiddleware(65-67)packages/server/src/middleware-utils.ts (1)
addMiddleware(29-31)packages/contract/src/error.ts (1)
mergeErrorMap(37-39)
🔇 Additional comments (8)
packages/server/src/middleware-decorated.ts (3)
15-20: Good addition: carry errorMap on decorated middlewaresAdding an optional errorMap to DecoratedMiddleware makes the merge story straightforward across mapInput/concat. Looks correct.
106-113: mapInput correctly preserves errorMapForwarding decorated.errorMap into the nested decoration keeps the metadata intact after input mapping. Nice.
125-137: Concat composition looks correct; verify context merge order under nestingThe next chaining merges contexts as { ...options.context, ...next1?.context } then later { ...next1?.context, ...next2?.context }. This maintains right-most precedence. Just flagging to double-check against your intended override order for nested middlewares.
Would you like a follow-up test that chains decorated.concat twice (with and without mapInput) and asserts both errorMap merge and context override order?
packages/server/src/builder.test.ts (1)
133-134: Test update matches new decorateMiddleware signatureExpecting decorateMiddleware(mid, def.errorMap) ensures propagation from builder into decoration. Looks good.
packages/server/src/procedure-decorated.ts (2)
15-16: Importing AnyDecoratedMiddleware aligns the public API with decorated inputsThis keeps use flexible without breaking existing plain middleware usage. Good.
140-151: Early return when no errorMap keeps hot path leanSkipping a merge when middleware.errorMap is falsy is a nice optimization and preserves prior behavior. LGTM.
packages/server/tests/error-middleware-pattern.test.ts (1)
26-46: Great coverage of the new automatic merge behaviorThis verifies the key runtime effect: defined=true after using a middleware that contributes its own errorMap. Nice assertion on router['~orpc'].errorMap as well.
packages/server/src/builder.ts (1)
149-150: Passing builder errorMap into decorateMiddleware is the right moveThis ensures decorated middlewares created via builder.middleware start with the router’s current errorMap. Matches the test update.
QzCurious
force-pushed
the
feat/make-error-thrown-from-middleware-defined-error
branch
from
adb581a to
1f74bc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/middleware-decorated.ts (1)
33-53: Public type signature of concat doesn’t accept DecoratedMiddleware (but implementation does) — update overloadsInterface overloads still accept only Middleware<…>; however, implementation at Line 119 accepts AnyMiddleware | AnyDecoratedMiddleware. As-is, callers won’t be able to pass a DecoratedMiddleware without casts, defeating the purpose of the change.
Apply this diff to both overloads to accept a decorated input with matching generics:
- >( - middleware: Middleware< - UInContext | MergedCurrentContext<TInContext, TOutContext>, - UOutContext, - UInput, - TOutput, - TErrorConstructorMap, - TMeta - >, + >( + middleware: + | Middleware< + UInContext | MergedCurrentContext<TInContext, TOutContext>, + UOutContext, + UInput, + TOutput, + TErrorConstructorMap, + TMeta + > + | DecoratedMiddleware< + UInContext | MergedCurrentContext<TInContext, TOutContext>, + UOutContext, + UInput, + TOutput, + TErrorConstructorMap, + TMeta + >,And for the mapInput overload:
- >( - middleware: Middleware< - UInContext | MergedCurrentContext<TInContext, TOutContext>, - UOutContext, - UMappedInput, - TOutput, - TErrorConstructorMap, - TMeta - >, + >( + middleware: + | Middleware< + UInContext | MergedCurrentContext<TInContext, TOutContext>, + UOutContext, + UMappedInput, + TOutput, + TErrorConstructorMap, + TMeta + > + | DecoratedMiddleware< + UInContext | MergedCurrentContext<TInContext, TOutContext>, + UOutContext, + UMappedInput, + TOutput, + TErrorConstructorMap, + TMeta + >,Also applies to: 61-83
♻️ Duplicate comments (1)
packages/server/src/middleware-decorated.ts (1)
96-109: Bug: Provided errorMap is overwritten by middleware.errorMap — merge instead with clear precedenceCurrent sequence sets decorated.errorMap = errorMap, then unconditionally overwrites it with middleware.errorMap if present, dropping the router/global map passed from Builder. Merge both; typically the middleware’s own map should override defaults on key collision.
Apply:
- // Attach error map if provided - if (errorMap) { - decorated.errorMap = errorMap - } - if ('errorMap' in middleware) { - decorated.errorMap = middleware.errorMap - } + // Merge provided errorMap with any existing one on the middleware. + // Precedence: middleware.errorMap overrides passed-in defaults on key collision. + const incoming = errorMap + const existing = ('errorMap' in middleware ? (middleware as AnyDecoratedMiddleware).errorMap : undefined) + const merged = existing || incoming ? { ...(incoming ?? {}), ...(existing ?? {}) } : undefined + if (merged) { + decorated.errorMap = merged + }
🧹 Nitpick comments (4)
packages/server/src/middleware-decorated.ts (4)
15-20: Nice: DecoratedMiddleware now carries an optional errorMapClear doc and scoping as internal. Consider documenting precedence semantics (who wins on key collision) in concat/mapInput JSDoc for future readers.
119-128: Safer merge for combinedErrorMap and clarify precedenceSpreading possibly undefined is brittle and the current code relies on JS semantics that can be surprising. Also, consider typing the result explicitly and reusing the same precedence rule (concatMiddleware’s map overrides earlier).
Apply:
- const combinedErrorMap = { - ...decorated.errorMap, - ...('errorMap' in concatMiddleware ? concatMiddleware.errorMap : undefined), - } + const left = decorated.errorMap + const right = ('errorMap' in concatMiddleware ? (concatMiddleware as AnyDecoratedMiddleware).errorMap : undefined) + const combinedErrorMap: ErrorMap | undefined = + left || right ? { ...(left ?? {}), ...(right ?? {}) } : undefined
129-141: Minor: simplify next() parameter handling for readabilityDestructuring into arrays for single optional params is unconventional and obscures intent. Optional params are clearer.
Apply:
- const concatted = decorateMiddleware((options, input, output, ...rest) => { - const merged = middleware({ - ...options, - next: (...[nextOptions1]: [any]) => mapped({ - ...options, - context: { ...options.context, ...nextOptions1?.context }, - next: (...[nextOptions2]) => options.next({ context: { ...nextOptions1?.context, ...nextOptions2?.context } }) as any, - }, input, output, ...rest), - } as any, input as any, output as any, ...rest) + const concatted = decorateMiddleware((options, input, output, ...rest) => { + const merged = middleware({ + ...options, + next: (nextOptions1?: any) => + mapped( + { + ...options, + context: { ...options.context, ...nextOptions1?.context }, + next: (nextOptions2?: any) => + options.next({ context: { ...nextOptions1?.context, ...nextOptions2?.context } }) as any, + }, + input, + output, + ...rest, + ), + } as any, input as any, output as any, ...rest)
15-20: Request: document and test error-map precedence rulesPlease add a short note in code (JSDoc) and a test asserting that when both router-level and middleware-level maps provide the same code, the middleware-level mapping wins (and similarly in concat, the later middleware’s mapping wins).
I can draft an additional test in packages/server/tests/error-middleware-pattern.test.ts that asserts precedence across: builder → middleware → concat(middleware2). Let me know if you want me to push that.
Also applies to: 96-109, 119-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/server/src/middleware-decorated.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/src/middleware-decorated.ts (3)
packages/contract/src/error.ts (1)
ErrorMap(31-33)packages/server/src/builder.ts (2)
middleware(146-150)input(251-259)packages/server/src/middleware.ts (3)
Middleware(46-61)AnyMiddleware(63-63)MapInputMiddleware(65-67)
🔇 Additional comments (4)
packages/server/src/middleware-decorated.ts (4)
1-1: Import looks correctBringing in ErrorMap from @orpc/contract is needed for the new property usage.
86-87: Good addition: AnyDecoratedMiddleware aliasThis will simplify Builder/Procedure typing and unions.
110-117: mapInput correctly preserves the errorMapPassing decorated.errorMap through decorateMiddleware keeps propagation intact across mapping.
140-141: LGTM: Passing combinedErrorMap to outer decorateMiddlewareEnsures merged maps persist when chaining further.
| * Error map associated with this middleware (if any) | ||
| * @internal | ||
| */ | ||
| errorMap?: ErrorMap |
There was a problem hiding this comment.
Your current implementation seem not "typesafe" at all, it only cover at runtime level while ignore the type level.
To fully support this, we require procedure inherit middleware's errors on both runtime and type levels when .use
And you should change the type of Middleware instead of DecoratedMiddleware
There was a problem hiding this comment.
For typesafe, do you mean we should be able to infer the errors type in handler? I do understand my implementation is not fulfilling it. But I want to fix the "defined for error thrown from middleware is false" thing first without affecting current behavior.
Screen.Recording.2025-08-24.at.17.mp4
If we want it work as a whole, that is, both "defined=true" and "typesafe", I agree change Middleware would be the right way to it. Then, do you mind if I store the errors as a property of a middleware?
I tried to change type of Middleware as I did with DecoratedMiddleware but it could affect more places. Wish I can make it well.
There was a problem hiding this comment.
You must make sure runtime errors and type errors are matching, if only merging at runtime the client can throw defined error more than the type describe -> unexpected behavior.
Fore reference here my implementation in the past: #224 - but it faces some issue with contract-first
Changes of the PR
This change ensures that errors thrown from middleware are properly recognized as "defined" errors, fixing the error handling issue where middleware errors were not being properly typed or handled.
I want #757 (comment) to be implement so I give it a try.
A simple snippet I used to test it:
It logs:
This PR make
definedto betrueSoem notes
I found that type of middleware already know errors but not really use it at the time. I tried not to touch too much not-directly-related-code and I think the best spot is to store errors in DecoratedMiddleware. As the author want middleware to be clean. And thank to this, the implementation is not affecting too much logical structure.
packages/server/tests/error-middleware-pattern.test.ts is a simple test to state out how thing should work before and after.
Because the test strictly want inline middleware to be as is
orpc/packages/server/src/builder.test.ts
Lines 150 to 160 in 7f288fb
It took me some time to make the code work and be pretty. I introduce a
AnyDecoratedMiddlewareto help with it.https://github.com/QzCurious/orpc/blob/d558620109300ba41f04984fe1713b9184d4ea7f/packages/server/src/builder.ts#L192-L212
Summary by CodeRabbit
New Features
Tests