refactor: impl Application and Agent extend with Class #65
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the scheduling system across multiple files. The changes primarily involve transitioning from object-based exports to class-based implementations, updating import paths for core types like Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Agent as Agent
participant Schedule as Schedule
participant Strategy as ScheduleStrategy
App->>Agent: Initialize
Agent->>Schedule: Create Schedule
Schedule->>Strategy: Create Strategy
App->>Schedule: start()
Schedule->>Strategy: start()
Strategy-->>Schedule: Started
Schedule-->>App: Ready
App->>Schedule: close()
Schedule->>Strategy: close()
Strategy-->>Schedule: Closed
Schedule-->>App: Closed
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/app/extend/application.ts (1)
4-4: Consider keeping the consistent Symbol description formatThe Symbol description was changed from 'application#scheduleWorker' to 'application scheduleWorker'. While this works functionally, consider maintaining the conventional '#' separator for consistency with other Symbol descriptions in the codebase.
src/app/extend/agent.ts (1)
27-31: Consider adding null check in schedule getter.While the lazy initialization pattern is good, consider adding a type guard to ensure schedule is never undefined after initialization.
get schedule() { - let schedule = this[SCHEDULE] as Schedule; + let schedule = this[SCHEDULE] as Schedule | undefined; if (!schedule) { this[SCHEDULE] = schedule = new Schedule(this); } return schedule; }src/app/extend/application.unittest.ts (1)
Line range hint
37-42: Enhance error handling with more specific error types.The current error handling could be improved by:
- Using a custom error class for schedule-related errors
- Providing more context in error messages
+ class ScheduleNotFoundError extends Error { + constructor(schedulePath: string) { + super(`Cannot find schedule ${schedulePath}`); + this.name = 'ScheduleNotFoundError'; + } + } try { schedule = this.scheduleWorker.scheduleItems[schedulePath]; if (!schedule) { - throw new TypeError(`Cannot find schedule ${schedulePath}`); + throw new ScheduleNotFoundError(schedulePath); } } catch (err: any) { err.message = `[@eggjs/schedule] ${err.message}`; throw err; }src/lib/strategy/base.ts (1)
25-27: Consider documenting the empty implementationThe empty async implementation suggests this is meant to be overridden by subclasses. Consider adding a JSDoc comment to clarify this intention.
+ /** + * Start the strategy. Override this method in subclasses to implement specific start behavior. + */ async start() { // empty loop by default }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/agent.ts(2 hunks)src/app.ts(1 hunks)src/app/extend/agent.ts(1 hunks)src/app/extend/application.ts(1 hunks)src/app/extend/application.unittest.ts(3 hunks)src/index.ts(1 hunks)src/lib/schedule.ts(3 hunks)src/lib/schedule_worker.ts(1 hunks)src/lib/strategy/base.ts(2 hunks)src/lib/strategy/timer.ts(2 hunks)src/lib/types.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/lib/types.ts
🔇 Additional comments (16)
src/index.ts (1)
1-8: LGTM! Clean and well-organized exports
The new imports and exports are well-structured and properly organized, maintaining a clear public API for the module. The use of .js extensions in imports follows the correct TypeScript ESM pattern.
src/app/extend/application.ts (1)
6-17: LGTM! Well-structured class implementation
The refactoring to a class-based implementation is clean and improves type safety. The scheduleWorker getter properly implements the singleton pattern with explicit type casting.
src/lib/schedule_worker.ts (2)
3-3: LGTM! Clean import update
The import path change correctly reflects the new class-based structure while maintaining type safety.
Line range hint 5-25: LGTM! Well-structured worker implementation
The ScheduleWorker class implementation is clean and follows best practices:
- Uses private fields appropriately
- Maintains clear method signatures
- Properly typed properties
src/app/extend/agent.ts (1)
8-14: LGTM! Clean transition to class-based implementation.
The class extension and getter implementation for ScheduleStrategy follows good practices by:
- Properly extending EggAgent
- Using clear JSDoc annotations
- Maintaining clean encapsulation through getters
src/agent.ts (2)
34-35: LGTM! Proper async handling.
The await on schedule.start() ensures proper initialization sequence.
38-42: LGTM! Clean shutdown implementation.
The beforeClose method properly handles the async shutdown of the scheduler.
src/app/extend/application.unittest.ts (1)
Line range hint 46-53: LGTM! Well-structured context handling.
The anonymous context creation and ctxStorage usage provides good isolation for schedule execution.
src/lib/strategy/base.ts (2)
1-3: LGTM: Import path update aligns with class-based refactoring
The import path change for Agent type reflects the architectural shift to class-based implementations.
29-31: Verify async close propagation
The close method is now async but maintains the same simple implementation. Let's verify this change is properly handled by all strategy implementations.
✅ Verification successful
Async close change is safely propagated
The verification shows that:
- There's only one strategy implementation (
TimerStrategy) that extendsBaseStrategy TimerStrategydoesn't override theclose()method, thus inheriting the async implementation fromBaseStrategy- The parent
Scheduleclass properly awaits the close call withawait instance.close()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all strategy implementations to verify they properly handle async close
ast-grep --pattern 'class $_ extends BaseStrategy {
$$$
close() {
$$$
}
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find strategy implementations
# First find all files that might contain strategy implementations
rg -l "extends BaseStrategy"
# Then let's look for close method implementations
rg "close\(\)" -A 5
Length of output: 1217
Script:
#!/bin/bash
# Let's examine the timer strategy implementation specifically
cat src/lib/strategy/timer.ts
Length of output: 3356
src/app.ts (1)
3-6: LGTM: Clean type import reorganization
The import restructuring improves code organization while maintaining type safety. The separation of framework types (ILifecycleBoot, EggLogger) from application types (Application) is a good practice.
src/lib/schedule.ts (2)
34-34: LGTM: Documentation improvement
Fixed typo in documentation comment from "speical" to "special".
94-99: LGTM: Proper async cleanup implementation
The close() method correctly awaits each instance's close() method, ensuring proper cleanup of resources.
src/lib/strategy/timer.ts (3)
Line range hint 11-106: Implementation looks solid!
The TimerStrategy class demonstrates:
- Robust error handling for cron expressions
- Safe handling of long intervals
- Clear separation of concerns
- Comprehensive logging
Line range hint 37-47: Consider the necessity of async implementation
The start() method has been made async but doesn't contain any await operations. While this might be for interface consistency, unnecessary async methods can impact performance and complicate error handling.
If the async implementation is not required by the base class or other strategies, consider removing the async keyword:
- async start() {
+ start() {✅ Verification successful
Based on the results, I can now generate the final response since I have found that the BaseStrategy class in src/lib/strategy/base.ts explicitly declares start() as an async method (line 25). Therefore, the implementation in the timer strategy is correctly following the base class contract.
The async implementation is required by the base class
The async implementation in the timer strategy's start() method is necessary as it implements the async contract defined in BaseStrategy. The base class declares async start() method, making it a requirement for all derived strategy classes to maintain the same async signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the BaseStrategy requires async implementation
# Test 1: Check the BaseStrategy class definition
ast-grep --pattern 'class BaseStrategy {
$$$
start() {
$$$
}
$$$
}'
# Test 2: Check for other strategy implementations
ast-grep --pattern 'class $_ extends BaseStrategy {
$$$
start() {
$$$
}
$$$
}'
Length of output: 4737
9-9: Verify the Agent type compatibility
The import path change from 'egg' to a local implementation aligns with the class-based refactoring. Let's verify the new type definition exists and maintains compatibility.
✅ Verification successful
Import path change is valid and type-safe
The import path change is correct and maintains type compatibility:
- The file
src/app/extend/agent.tsexists and exports anAgentclass - The exported
Agentclass properly extendsEggAgentfrom 'egg' - Multiple files in the codebase consistently use this new import path
- The class provides all necessary schedule-related functionality through its properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Agent type definition exists and check its interface
# Test 1: Check if the file exists
fd "agent.js$" "src/app/extend"
# Test 2: Check the Agent class definition
ast-grep --pattern 'class Agent {
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's try a different approach to verify the Agent type:
# Check for TypeScript definition files for Agent
fd "agent.ts$|agent.d.ts$" "src/app/extend"
# Look for Agent type/interface/class definitions in TypeScript files
rg "(?:interface|type|class)\s+Agent" -t ts
# Check if the import path exists (including .ts extension)
fd "agent.(js|ts)$" "src/app/extend"
# Look for Agent-related exports
rg "export.*(Agent)" -t ts
Length of output: 693
Script:
#!/bin/bash
# Let's check the Agent class implementation to verify type compatibility
ast-grep --pattern 'export default class Agent extends EggAgent {
$$$
}'
# Also check the EggAgent import
rg "import.*EggAgent" -t ts src/app/extend/agent.ts
Length of output: 1352
| async start() { | ||
| debug('start'); | ||
| this.closed = false; | ||
| for (const instance of this.#strategyInstanceMap.values()) { | ||
| instance.start(); | ||
| } |
There was a problem hiding this comment.
Critical: start() method should await instance.start()
The start() method is marked async but doesn't await the instance.start() calls. This could lead to race conditions if strategy implementations rely on async initialization.
async start() {
debug('start');
this.closed = false;
for (const instance of this.#strategyInstanceMap.values()) {
- instance.start();
+ await instance.start();
}
}📝 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.
| async start() { | |
| debug('start'); | |
| this.closed = false; | |
| for (const instance of this.#strategyInstanceMap.values()) { | |
| instance.start(); | |
| } | |
| async start() { | |
| debug('start'); | |
| this.closed = false; | |
| for (const instance of this.#strategyInstanceMap.values()) { | |
| await instance.start(); | |
| } |
| async start() { | ||
| debug('start'); | ||
| this.closed = false; | ||
| for (const instance of this.#strategyInstanceMap.values()) { | ||
| instance.start(); | ||
| } | ||
| } | ||
|
|
||
| close() { | ||
| async close() { | ||
| this.closed = true; | ||
| for (const instance of this.#strategyInstanceMap.values()) { | ||
| instance.close(); | ||
| await instance.close(); | ||
| } | ||
| debug('close'); |
There was a problem hiding this comment.
💡 Codebase verification
Missing await for async start() call in Schedule class
The start() method in BaseStrategy is declared as async, but it's not being awaited in the Schedule class's start() method. This needs to be fixed for proper async/await consistency:
- In
src/lib/schedule.ts, line 90:instance.start()should beawait instance.start()
🔗 Analysis chain
Verify consistent async/await usage across the codebase
Let's ensure all async method calls are properly awaited throughout the codebase.
Based on the results, I can see that instance.start() in schedule.ts is not awaited while the method is declared as async. Let me verify if the strategy instances' start methods are also async.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find potential missing await keywords for start/close method calls
rg -A 2 "\\.(start|close)\\(\\)" --type ts
# Find all async method declarations to cross-reference
ast-grep --pattern 'async $_($$) {
$$$
}'
Length of output: 704
Script:
#!/bin/bash
# Check if strategy instances have async start methods
ast-grep --pattern 'class $_ {
$$$
async start() {
$$$
}
$$$
}'
# Also check for non-async start methods to be thorough
ast-grep --pattern 'class $_ {
$$$
start() {
$$$
}
$$$
}'
Length of output: 18415
Summary by CodeRabbit
Release Notes
New Features
beforeCloseto manage application shutdown.Schedule,BaseStrategy, andTimerStrategy.Bug Fixes
initmethod of theScheduleclass.Refactor
Chores
AgentandApplicationtypes across multiple files.src/index.tsto include new modules.