-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: make Context state property can be override by Sub Class #14
Conversation
WalkthroughThis pull request introduces several modifications to the Koa framework's core components. The changes primarily focus on enhancing the Changes
Sequence DiagramsequenceDiagram
participant App as Koa Application
participant Context as Custom Context
participant Middleware as Middleware
App->>Context: Create Context
Context-->>App: Initialize with empty state
Middleware->>Context: Modify state
Context-->>Middleware: Update state
Middleware->>App: Continue request processing
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
example/extend/Context.ts (1)
4-6
: Add return type annotation for better type safety.Consider adding explicit return type annotation to the state getter:
- get state() { + get state(): Record<string, any> { return { foo: 'bar' }; }test/context/state.test.ts (2)
34-36
: Improve type safety by avoiding type assertion.Instead of using type assertion, consider defining a proper interface for the state:
- assert.deepStrictEqual(ctx.state, { foo: 'bar' }); - (ctx.state as any).user = 'example'; + interface CustomState extends Record<string, string> { + foo: string; + user?: string; + } + const state = ctx.state as CustomState; + assert.deepStrictEqual(state, { foo: 'bar' }); + state.user = 'example';
25-42
: Add test cases for additional scenarios.Consider adding tests for:
- Nested state properties
- Multiple middleware state modifications
- Error cases (e.g., attempting to modify immutable state)
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
example/extend/Context.ts
(1 hunks)src/context.ts
(2 hunks)src/request.ts
(2 hunks)test/application/index.test.ts
(1 hunks)test/context/state.test.ts
(1 hunks)
🔇 Additional comments (4)
example/extend/Context.ts (1)
4-6
: Consider implications of immutable state implementation.
The current implementation returns a new object on every state access. This means state modifications won't persist across middleware. If this is intentional, consider documenting this behavior.
src/context.ts (1)
22-22
: LGTM! Well-structured encapsulation of the state property.
The changes effectively make the state property overridable by subclasses while maintaining proper encapsulation through the private field pattern. This is a good implementation that follows TypeScript best practices.
Also applies to: 222-224
src/request.ts (2)
15-17
: LGTM! Good type definition for the socket interface.
The RequestSocket interface properly extends the Socket type and explicitly defines the encrypted property, improving type safety and maintainability.
306-306
: LGTM! Clean type implementation in the socket getter.
The change simplifies the type casting by utilizing the newly defined RequestSocket interface, improving code readability while maintaining the same functionality.
const server = app.listen(); | ||
|
||
return request(server) | ||
.get('/') | ||
.expect(404); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add server cleanup after test completion.
The server should be closed after the test completes to prevent resource leaks:
const server = app.listen();
return request(server)
.get('/')
- .expect(404);
+ .expect(404)
+ .then(() => server.close());
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const server = app.listen(); | |
return request(server) | |
.get('/') | |
.expect(404); | |
}); | |
const server = app.listen(); | |
return request(server) | |
.get('/') | |
.expect(404) | |
.then(() => server.close()); | |
}); |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 1718 1725 +7
Branches 319 321 +2
=========================================
+ Hits 1718 1725 +7 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [2.20.2](v2.20.1...v2.20.2) (2024-12-19) ### Bug Fixes * make Context state property can be override by Sub Class ([#14](#14)) ([49abe7f](49abe7f))
Summary by CodeRabbit
New Features
CustomContext
class for enhanced state management.RequestSocket
interface to simplify socket type definitions.Bug Fixes
Context
class.Tests
ctx.state
functionality and Koa application context logging.