chore: 1.2.11 #5610
chore: 1.2.11 #5610
Conversation
Replace Node.js EventEmitter with Bun's native EventTarget implementation to align with the project's Bun-first approach and reduce Node.js dependencies. Changes: - Convert InternalMessageBus to use EventTarget with EventEmitter-like API - Update SimpleMigrationAgent to extend EventTarget instead of EventEmitter - Maintain backward compatibility with existing event handling interfaces - Add wrapper methods (on/off/emit) to preserve existing API contracts This migration supports the project's goal of eliminating Node.js-specific dependencies in favor of Bun's native APIs. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Document that EventEmitter should never be used due to Bun compatibility issues. Add migration guidelines and examples for using Bun's native EventTarget API instead. This ensures future development maintains Bun compatibility and avoids Node.js-specific APIs that may cause issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Implemented off, removeListener, removeAllListeners, listenerCount, and listeners methods in SimpleMigrationAgent for improved event handler management and EventEmitter compatibility. Also updated InternalMessageBus to prevent duplicate event handler registration.
Updated the emit methods in SimpleMigrationAgent and InternalMessageBus to return the result of dispatchEvent. This change improves consistency and allows callers to know if the event was canceled.
Updated event handler wrappers in SimpleMigrationAgent and InternalMessageBus to check if the event is a CustomEvent before accessing the detail property. This prevents potential runtime errors when non-CustomEvent objects are passed to the handler.
feat: migrate from EventEmitter to Bun native EventTarget API
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Critical Issues❌ CRITICAL: Missing tests for SimpleMigrationAgent → Fix: Add comprehensive test file at
❌ CRITICAL: No tests for EventTarget compatibility → Fix: Add tests verifying EventEmitter → EventTarget migration maintains backward compatibility for existing consumers Important Issues
const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;Should be: const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers!.delete(handler);The Suggestions💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments to EventTarget methods explaining the migration compatibility layer 💡 SUGGESTION: Event handler memory management → Consider: Add cleanup in class destructors to prevent memory leaks from lingering event handlers 💡 SUGGESTION: Performance optimization → Consider: The Map-based handler tracking in both files could be optimized for large numbers of listeners What's Working Well✅ Implementation follows CLAUDE.md EventTarget migration guidelines VerdictBLOCK - Missing critical test coverage. The SimpleMigrationAgent class needs comprehensive tests before this can be merged to main. |
Removed dependencies and lockfile entries for @elizaos/plugin-anthropic, @elizaos/plugin-discord, @elizaos/plugin-openai, and @elizaos/plugin-coinmarketcap, along with their related packages. This reduces the project footprint and eliminates unused or deprecated plugins.
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Critical Issues❌ CRITICAL: Missing tests for SimpleMigrationAgent → Fix: Add comprehensive test file at
❌ CRITICAL: No tests for EventTarget compatibility → Fix: Add tests verifying EventEmitter → EventTarget migration maintains backward compatibility for existing consumers Important Issues
const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;Should be: const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers!.delete(handler);The Suggestions💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments to EventTarget methods explaining the migration compatibility layer 💡 SUGGESTION: Event handler memory management → Consider: Add cleanup in class destructors to prevent memory leaks from lingering event handlers 💡 SUGGESTION: Performance optimization → Consider: The Map-based handler tracking in both files could be optimized for large numbers of listeners What's Working Well✅ Implementation follows CLAUDE.md EventTarget migration guidelines VerdictBLOCK - Missing critical test coverage. The SimpleMigrationAgent class needs comprehensive tests before this can be merged to main. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Replace 'any' with 'unknown' in simple-migration-agent.ts and bus.ts - Replace generic Function type with explicit function signatures - Fixes CodeFactor maintainability issues 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
fix: replace 'any' types with 'unknown' and explicit function types
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Critical Issues❌ CRITICAL: Missing tests for SimpleMigrationAgent → Fix: Add comprehensive test file at
❌ CRITICAL: Method chaining broken → Fix: Both on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: No tests for EventTarget compatibility → Fix: Add tests verifying EventEmitter → EventTarget migration maintains backward compatibility for existing consumers Important Issues
const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers!.delete(handler);
}The Suggestions💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments to EventTarget methods explaining the migration compatibility layer 💡 SUGGESTION: Event handler memory management → Consider: Add cleanup in class destructors to prevent memory leaks from lingering event handlers 💡 SUGGESTION: Performance optimization → Consider: The Map-based handler tracking in both files could be optimized for large numbers of listeners What's Working Well✅ Implementation follows CLAUDE.md EventTarget migration guidelines VerdictBLOCK - Missing critical test coverage. The SimpleMigrationAgent class needs comprehensive tests before this can be merged to main. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- SimpleMigrationAgent.on() now returns 'this' for method chaining - InternalMessageBus.on() now returns 'this' for method chaining - Maintains backward compatibility with EventEmitter-style chaining
fix: add method chaining support to EventTarget-based on() methods
|
Claude finished @wtfsayo's task —— View job I'll analyze this EventEmitter → EventTarget migration PR and provide a comprehensive review. Todo List:
Critical Issues❌ CRITICAL: Missing tests for SimpleMigrationAgent → Fix: Add comprehensive test file at
❌ CRITICAL: Method chaining broken → Fix: Both on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: No tests for EventTarget compatibility → Fix: Add tests verifying EventEmitter → EventTarget migration maintains backward compatibility for existing consumers Important Issues
const wrappedHandler = ((e: Event) => {
if (e instanceof CustomEvent) {
handler(e.detail);
} else {
handler(undefined);
}
}) as EventListener;
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers!.delete(handler);
}The Suggestions💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments to EventTarget methods explaining the migration compatibility layer 💡 SUGGESTION: Event handler memory management → Consider: Add cleanup in class destructors to prevent memory leaks from lingering event handlers 💡 SUGGESTION: Performance optimization → Consider: The Map-based handler tracking in both files could be optimized for large numbers of listeners What's Working Well✅ Implementation follows CLAUDE.md EventTarget migration guidelines VerdictBLOCK - Missing critical test coverage. The SimpleMigrationAgent class needs comprehensive tests before this can be merged to main. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…get implementations - Add unit tests for bus.ts EventEmitter compatibility in server package - Add unit tests for SimpleMigrationAgent EventEmitter compatibility in CLI package - Test all EventEmitter-like methods (emit, on, off, removeListener, etc.) - Verify proper memory management and handler cleanup - Test edge cases like duplicate handlers, undefined data, and error handling - Add documentation explaining the compatibility pattern and test coverage These tests ensure that EventTarget-based implementations maintain backward compatibility with existing code expecting EventEmitter APIs while following ElizaOS guidelines. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Introduces a removeAllListeners method to InternalMessageBus for removing all or specific event listeners, improving test isolation and API compatibility. Updates tests to use this method and enhances error handling verification for event listeners that throw exceptions.
test: add EventEmitter compatibility tests for EventTarget implementations
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
Refactors test files for EventEmitter compatibility to use consistent code formatting, including multi-line arrow functions and improved spacing. Updates the EventEmitter-Compatibility-README.md for clarity and formatting. No functional changes to test logic.
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add null safety check in InternalMessageBus off() method - Add documentation explaining why classes are used (EventTarget requirement) - Document intentional EventEmitter behavior deviation (duplicate prevention) - Improve type safety with optional chaining in cleanup logic
fix: address EventTarget migration PR review issues
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
Standardized whitespace in doc comments for consistency in simple-migration-agent.ts and bus.ts. No functional changes were made.
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
Updated all internal @elizaos/* dependencies in package.json files to use the "workspace:*" protocol for better monorepo management and consistency. Also added 'bun' as a dependency in plugin-bootstrap. This change helps ensure local packages are always resolved from the workspace.
|
Claude finished @wtfsayo's task —— View job PR Review: EventEmitter → EventTarget MigrationTodo List:
Review SummaryThis PR successfully migrates from Node.js EventEmitter to Bun's native EventTarget API with comprehensive test coverage. The implementation maintains backward compatibility while addressing Bun compatibility issues. Critical Issues❌ CRITICAL: Missing return type on on(event: string, handler: (data?: unknown) => void): this {
// existing implementation
return this;
}❌ CRITICAL: Type safety in SimpleMigrationAgent → Fix: In on(event: string, handler: (data?: unknown) => void): this {❌ CRITICAL: Classes violate CLAUDE.md guidelines → Fix: CLAUDE.md requires functional code patterns, but both Important Issues
if (wrappedHandler) {
this.removeEventListener(event, wrappedHandler);
eventHandlers?.delete(handler);
}
Minor Issues💡 SUGGESTION: Add JSDoc documentation → Consider: Add comprehensive JSDoc comments explaining the EventTarget compatibility layer and migration rationale. 💡 SUGGESTION: Performance optimization → Consider: Map-based handler tracking could be optimized for high-frequency listener operations. 💡 SUGGESTION: Error handling enhancement → Consider: Add try-catch blocks around handler execution for better error recovery. What's Working Well✅ Excellent test coverage - Both classes have comprehensive test suites covering EventEmitter compatibility Test Coverage Analysis✅ SimpleMigrationAgent tests - 423 lines of comprehensive EventEmitter compatibility tests Architecture Compliance✅ Uses bun test - All tests use VerdictAPPROVE with fixes - This is a well-implemented EventTarget migration with excellent test coverage. The critical issues are straightforward fixes that don't impact the core architecture. |
This pull request introduces a migration from Node.js
EventEmitterto Bun's nativeEventTargetAPI across the codebase to address compatibility issues with Bun. It includes updates to documentation, refactors key classes, and ensures backward compatibility by implementing anEventEmitter-like API on top ofEventTarget.Migration to
EventTargetAPI:Documentation Update: Added a new section in
CLAUDE.mdexplaining whyEventEmittershould not be used with Bun and providing a detailed migration guide, including an example of how to refactor code to useEventTarget.Refactor in
SimpleMigrationAgent:EventEmitterwithEventTargetinpackages/cli/src/utils/upgrade/simple-migration-agent.ts.emit,on,off, and aliases likeremoveListenerandremoveAllListenersto maintain anEventEmitter-like API. [1] [2] [3]Refactor in
InternalMessageBus:packages/server/src/bus.tsto useEventTargetinstead ofEventEmitter.emit,on,off,setMaxListeners) to provide a familiar API while leveragingEventTarget.Dependency Update:
turbopackage version from^2.5.4to^2.5.5inpackage.json.