fix: let MiddlewareFunc can extend by custom Context interface #13
Conversation
WalkthroughThis pull request introduces a custom middleware extension mechanism in a web framework. The changes focus on enhancing type flexibility and context handling across multiple files. The primary modifications include adding a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware
participant CustomContext
participant NextMiddleware
Client->>Middleware: Request
Middleware->>CustomContext: Create context
Middleware->>CustomContext: Access custom getter
Middleware->>NextMiddleware: Call next middleware
NextMiddleware-->>Middleware: Return
Middleware-->>Client: Response
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 1718 1718
Branches 319 319
=========================================
Hits 1718 1718 ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (3)
example/extend/middleware.ts (2)
3-8: LGTM: Well-structured custom context implementationThe
CustomContextclass properly extends the baseContextclass and demonstrates how to add custom properties using getters.Consider adding JSDoc comments to document the purpose of the
helloproperty:class CustomContext extends Context { // Add your custom properties and methods here + /** Example property that always returns 'world' */ get hello() { return 'world'; } }
12-16: Consider improving logging and error handlingThe middleware implementation demonstrates the usage of custom properties but could benefit from some improvements.
Consider these enhancements:
export const middleware: MiddlewareFunc<ICustomContext> = async (ctx, next) => { - console.log('middleware start, %s', ctx.hello, ctx.writable); + const requestId = ctx.get('x-request-id') || 'unknown'; + ctx.logger?.info('middleware start', { requestId, hello: ctx.hello, writable: ctx.writable }); try { await next(); - console.log('middleware end'); + ctx.logger?.info('middleware end', { requestId }); + } catch (err) { + ctx.logger?.error('middleware error', { requestId, error: err }); + throw err; + } };src/application.ts (1)
23-24: Well-designed type flexibility for middleware functions!The generic type implementation elegantly solves the custom context extension requirement while maintaining backward compatibility through the default type. This change:
- Allows custom context types in middleware functions
- Preserves existing behavior for code not using custom contexts
- Follows TypeScript best practices for generic types
Consider documenting this extensibility feature in the framework's documentation with examples of how to:
- Create custom context interfaces
- Implement middleware with custom contexts
- Type-safely compose middleware with different context types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/extend/middleware.ts(1 hunks)src/application.ts(2 hunks)src/context.ts(3 hunks)src/types.ts(1 hunks)
🔇 Additional comments (7)
src/types.ts (1)
10-10: LGTM: Type enhancement enables both string and symbol keys
The change to allow both string and symbol keys in AnyProto is a good improvement that enables more flexible property access patterns while maintaining type safety.
Let's verify the usage of this type:
✅ Verification successful
LGTM: Type enhancement is correctly implemented and used
The change to allow both string and symbol keys in AnyProto is well-aligned with the codebase implementation. The type is used consistently in both Context and Application classes, which both implement matching index signatures ([key: symbol | string]). This enables proper type safety for both runtime string keys and compile-time symbol keys across the framework's core components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of AnyProto to ensure compatibility
rg "AnyProto" --type ts
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual usage patterns in context.ts and application.ts
ast-grep --pattern 'request: $_ & AnyProto' -l ts
ast-grep --pattern 'response: $_ & AnyProto' -l ts
# Check for any symbol key usage in these files
rg "Symbol\." src/context.ts src/application.ts -A 2
Length of output: 159
Script:
#!/bin/bash
# Let's check the implementation details of these files to understand the usage
cat src/context.ts
cat src/application.ts
Length of output: 17124
src/context.ts (3)
14-14: LGTM: Index signature change aligns with AnyProto
The change to allow both string and symbol keys in the Context class is consistent with the AnyProto changes and enables proper typing for custom properties.
161-163: LGTM: Improved error handling readability
The addition of braces in the conditional block improves code readability while maintaining the same functionality.
287-287: LGTM: Fixed ContextDelegation type syntax
The syntax improvement in the ContextDelegation type definition enhances readability without changing the type behavior.
example/extend/middleware.ts (1)
10-10: LGTM: Proper type composition
The ICustomContext type correctly combines the custom context with the delegated properties.
src/application.ts (2)
14-14: LGTM! Explicit type import improves clarity.
The explicit import of ContextDelegation type makes the dependencies clearer and aligns with TypeScript best practices.
23-24: Verify the impact on existing middleware and context usage
The type changes could affect existing middleware implementations and context handling.
Let's check for potential issues:
Also applies to: 224-224
✅ Verification successful
Let me gather more information about the middleware usage patterns to ensure we have a complete picture.
Type changes are safe and backward compatible
The changes to middleware types are safe because:
- The
MiddlewareFunctype maintains backward compatibility by usingContextDelegationas the default type parameter - Existing middleware implementations in the codebase follow the correct patterns, as seen in
example/extend/middleware.ts - The type changes only add type safety through delegation without breaking existing context usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing middleware implementations and context usage that might be affected
# Look for middleware implementations
echo "Checking middleware implementations..."
ast-grep --pattern 'use($$$)' || true
# Look for createContext overrides
echo "Checking createContext overrides..."
ast-grep --pattern 'createContext($$$)' || true
# Look for direct ContextDelegation usage
echo "Checking ContextDelegation usage..."
rg "ContextDelegation" || true
Length of output: 2300
Script:
#!/bin/bash
# Check for middleware function implementations and their type signatures
echo "Checking middleware function patterns..."
ast-grep --pattern 'async function $_($ctx: $_, $next: $_) {
$$$
}'
echo "Checking arrow function middleware patterns..."
ast-grep --pattern '($ctx: $_, $next: $_) => {
$$$
}'
# Look for middleware type usage in function parameters
echo "Checking MiddlewareFunc usage..."
rg "MiddlewareFunc" -A 2
Length of output: 1767
| @@ -222,7 +221,7 @@ export class Application extends Emitter { | |||
| */ | |||
| protected createContext(req: IncomingMessage, res: ServerResponse) { | |||
| const context = new this.ContextClass(this, req, res); | |||
| return context as ContextDelegation; | |||
| return context; | |||
There was a problem hiding this comment.
Avoid implicit 'any' return type in createContext
The current implementation loses type safety by implicitly returning 'any'. While this enables custom contexts, it's too permissive and could lead to runtime errors.
Consider making the Application class generic and adding proper type constraints:
- export class Application extends Emitter {
+ export class Application<T extends ContextDelegation = ContextDelegation> extends Emitter {
// ... other code ...
- protected createContext(req: IncomingMessage, res: ServerResponse) {
+ protected createContext(req: IncomingMessage, res: ServerResponse): T {
- const context = new this.ContextClass(this, req, res);
+ const context = new this.ContextClass(this, req, res) as T;
return context;
}
}This change would:
- Maintain type safety while supporting custom contexts
- Ensure the return type matches the expected context type
- Provide better type inference throughout the application
Committable suggestion skipped: line range outside the PR's diff.
[skip ci] ## [2.20.1](v2.20.0...v2.20.1) (2024-12-19) ### Bug Fixes * let MiddlewareFunc can extend by custom Context interface ([#13](#13)) ([320fbf5](320fbf5))
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor