fix: allow sub class to override event methods #24
Conversation
WalkthroughThe pull request introduces updates to the project's CI workflow, package scripts, and test suite. The GitHub Actions workflow now includes Node.js version 23 for testing. A new Changes
Sequence DiagramsequenceDiagram
participant Client as SomeServiceClient2
participant Base as Base Class
participant Listener as Event Listener
Client->>Base: Extends Base class
Client->>Base: Override on() method
Client->>Listener: Add error listener
Client->>Base: Track listener arguments
Base->>Listener: Trigger event handling
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 (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #24 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 117 117
Branches 29 30 +1
=========================================
Hits 117 117 ☔ 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: 0
🧹 Nitpick comments (3)
test/index.test.ts (3)
9-17: LGTM! Consider adding type safety improvementsThe implementation correctly extends the Base class and properly maintains the parent functionality. However, we could improve type safety.
Consider adding proper types for the event arguments:
- protected _lists: any[] = []; + protected _lists: Array<[string, (...args: any[]) => void]> = []; - on(...args: any[]) { + on(event: string, listener: (...args: any[]) => void) { - this._lists.push(args); + this._lists.push([event, listener]); - super.on(args[0], args[1]); + super.on(event, listener); return this; }
20-24: LGTM! Consider adding more test coverageThe test correctly verifies that subclasses can override the
onmethod while maintaining the base functionality.Consider adding these additional test cases to improve coverage:
- Verify that
_listscontains the correct event registration- Test other event methods (e.g.,
once,addListener)- Test event removal through
offorremoveListenerExample:
it('should track event registrations correctly', () => { const c = new SomeServiceClient2(); const handler = () => {}; c.on('test', handler); assert.deepEqual(c._lists[0], ['test', handler]); });
9-24: Solid enhancement to event handling extensibilityThe implementation successfully enables subclasses to extend event handling while maintaining backward compatibility. This enhancement aligns well with the Open/Closed principle, allowing for behavior extension without modifying existing code.
This pattern could be useful in several scenarios:
- Event logging/monitoring
- Event transformation
- Conditional event handling
- Event debugging
Consider documenting this capability in the README to help other developers understand how to properly extend event handling in their subclasses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/nodejs.yml(1 hunks)package.json(1 hunks)src/index.ts(1 hunks)test/index.test.ts(1 hunks)
🔇 Additional comments (3)
src/index.ts (1)
48-50: LGTM! Good fix for allowing subclass event method overrides.
The change from this.on to super.on is a solid improvement that:
- Allows subclasses to safely override the
onmethod without breaking error handling - Maintains the error handling functionality by registering directly on the parent class
- Aligns with the principle of extensibility
package.json (1)
44-44: LGTM! Consistent test configuration.
The new test-local script maintains consistency with the CI test timeout, which is good for development and testing parity.
.github/workflows/nodejs.yml (1)
14-14: LGTM! Verify event handling in Node.js 23.
Adding Node.js 23 to the test matrix is good for ensuring compatibility. Since this PR modifies event handling behavior, it's particularly important to verify it works correctly across all Node.js versions.
Let's verify the event handling implementation is compatible with Node.js 23:
✅ Verification successful
LGTM! Event handling implementation is compatible with Node.js 23
The event handling implementation uses standard Node.js EventEmitter APIs that are stable across versions:
- Uses core
EventEmittermethods (on,once,emit) without version-specific features - Implements wrapper methods that only add async function support
- No usage of experimental or deprecated event APIs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any Node.js version-specific event handling APIs or changes
# Search for event-related APIs usage
rg -l 'EventEmitter|emit|on\(|once\(' | while read -r file; do
echo "=== $file ==="
rg 'EventEmitter|emit|on\(|once\(' "$file"
done
Length of output: 3807
[skip ci] ## [5.0.1](v5.0.0...v5.0.1) (2024-12-22) ### Bug Fixes * allow sub class to override event methods ([#24](#24)) ([d1ffb61](d1ffb61))
Summary by CodeRabbit
New Features
test-local) in the project.SomeServiceClient2) to enhance event handling capabilities.Bug Fixes
Tests