feat: remove EventEmitter dependency to support Edge Runtime#219
feat: remove EventEmitter dependency to support Edge Runtime#219ishymko merged 12 commits intoa2aproject:mainfrom
Conversation
Summary of ChangesHello @drew-foxall, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the SDK's portability and compatibility by refactoring its event handling mechanism to align with web standards. By migrating from Node.js's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, enabling support for modern edge runtimes by replacing the Node.js-specific EventEmitter with the web-standard EventTarget. The implementation of the compatibility layer is well done, and the addition of exported utilities for framework adapters is a thoughtful enhancement. My review includes a couple of suggestions to improve type safety in the new DefaultExecutionEventBus implementation, addressing potential runtime errors.
25da08b to
f7a21e7
Compare
f7a21e7 to
98e4e5f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in making the SDK compatible with modern edge runtimes by replacing the Node.js-specific EventEmitter with the web-standard EventTarget. The implementation provides a backward-compatible API, which is excellent for a smooth transition. I've added a few review comments on the new DefaultExecutionEventBus implementation, highlighting a high-severity type safety issue that could lead to runtime errors, a medium-severity issue regarding the correctness of the CustomEvent polyfill, and another medium-severity issue about a potential memory leak in the once method. Addressing these points will make the new implementation more robust and reliable.
|
Hi @drew-foxall, thank you for the PR and for maintaining the fork compatible with Edge runtime! Seems like it's relatively easy to achieve compatibility by dropping this single dependency, I like this idea. Two small things:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the event bus to use the web-standard EventTarget API instead of Node.js's EventEmitter, which is a great move for enabling native support on edge runtimes. The implementation of DefaultExecutionEventBus is well-crafted to maintain backward compatibility with the EventEmitter API, and the new test suite is comprehensive, covering many edge cases. My review includes a couple of minor suggestions to enhance type safety and code cleanliness.
1851e50 to
9e5ae16
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the SDK's compatibility by replacing the Node.js-specific EventEmitter with the web-standard EventTarget. The implementation is well-executed, preserving the familiar EventEmitter API, and is backed by a thorough new test suite. My feedback focuses on improving the documentation around memory management to ensure developers use the new event bus correctly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of replacing the Node.js-specific EventEmitter with the web-standard EventTarget, which is a crucial step for enabling edge runtime support. The new DefaultExecutionEventBus implementation is thorough and the addition of a comprehensive test suite is excellent. One point of clarification: the pull request description mentions using WeakMap for listener tracking, but the implementation correctly uses Map to maintain strong references, which is consistent with EventEmitter's behavior. This is the right choice for compatibility, and it might be worth updating the description to reflect the implementation. I've added a few suggestions to improve the robustness of the event handling logic and the test suite.
0f14e65 to
1e866b1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the SDK's portability by removing the Node.js events dependency and replacing it with the web-standard EventTarget. The new DefaultExecutionEventBus implementation is robust, well-thought-out, and impressively mimics the EventEmitter API, including subtle behaviors like LIFO listener removal. The addition of comprehensive tests provides strong confidence in the correctness of this change. I have a couple of suggestions to reduce code duplication and improve maintainability, but overall this is a high-quality contribution.
|
@ishymko thanks for the review ^-^ I've tried to address your and Gemini's feedback Looking forward to the next release and narrowing my fork down to just edge runtime support (and a little ecosystem around them) |
1e866b1 to
f936612
Compare
ecd1715 to
ca8dc12
Compare
ece30f4 to
1b6ba8b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to improve edge runtime compatibility by replacing the Node.js-specific EventEmitter with the web-standard EventTarget. The implementation is thorough, correctly replicating EventEmitter semantics like LIFO removal and FIFO dispatch, and is accompanied by an extensive new test suite. My review focuses on a few areas to further enhance type safety, code clarity, and test robustness.
This PR addresses issue a2aproject#218 by making two key changes: ## 1. Replace EventEmitter with EventTarget - Removes Node.js 'events' dependency - Uses web-standard EventTarget API available in all modern runtimes - Maintains backward-compatible API (on, off, once, removeAllListeners) - Enables native support for: - Cloudflare Workers (no nodejs_compat needed) - Deno (no npm: shims required) - Bun (native web APIs) - Browsers (true universal JavaScript) - Node.js 15+ (EventTarget built-in) ## 2. Export extension utilities for framework adapters - Exports HTTP_EXTENSION_HEADER constant - Exports getRequestedExtensions() utility function - Enables community framework adapters (Hono, Fastify, Koa, etc.) to properly handle A2A extensions without duplicating code ## Breaking Changes - Requires Node.js 15+ (EventTarget support) - Node.js 14 no longer supported (EOL anyway) ## Testing All 146 existing tests pass with these changes. Closes a2aproject#218
- Fix type safety: use function overloads for ExecutionEventBus - 'event' listeners receive EventListener (event: AgentExecutionEvent) => void - 'finished' listeners receive FinishedListener () => void - Fix CustomEvent polyfill to default detail to null per web spec - Document once() listener lifecycle (matches EventEmitter behavior) - Remove extension utilities export (to be handled in separate PR) - Update samples/extensions to use new typed interface
The previous implementation used a WeakMap that stored a single wrapped listener per original listener. When the same listener was registered multiple times, the map entry was overwritten, leaving orphaned listeners that could not be removed via off(). Changes: - Use Map<eventName, Map<listener, wrappedListener[]>> to track all wrapped listeners per (eventName, listener) pair - off() now removes listeners in LIFO order (matching EventEmitter) - once() properly cleans up its specific wrapped listener when fired - removeAllListeners() iterates all wrapped listeners for cleanup
Tests cover: - Basic on/off/once functionality - Multiple registrations of same listener - LIFO removal order (matching EventEmitter) - once() auto-cleanup after firing - removeAllListeners() for single event and all events - Event firing order (FIFO) - Mixed on() and once() usage - Method chaining
- Use e.type instead of 'detail' in e for event type detection (more explicit) - Use counter instead of Date.now() for test message IDs (more reliable) - Clarify JSDoc: drop-in replacement with identical EventEmitter semantics - Remove redundant afterEach cleanup (new instance created each test)
Address maintainer feedback: restore original ExecutionEventBus interface signature to prevent build-time breaking changes for users implementing the interface or extending DefaultExecutionEventBus. - Removed EventListener/FinishedListener type exports - Restored original method signature: (eventName: 'event' | 'finished', listener: (event: AgentExecutionEvent) => void) - Reverted extensions.ts sample to original (used as compatibility test) - Internal EventTarget implementation preserved with identical behavior
- Replace unsafe type casts with type guards and fail-fast validation - Use separate internal storage for 'event' and 'finished' listeners - Preserve 'this' context (bound to event bus) matching EventEmitter semantics - Use Function.prototype.call for type-safe invocation without casts - Reorder validation before cleanup in once() handlers - Add 4 tests verifying 'this' context for all listener types Internal implementation uses: - Type guard isAgentExecutionCustomEvent() with error on failure - listener.call(this, ...) to preserve EventEmitter 'this' binding - Separate Maps for eventListeners and finishedListeners Total tests: 199 (27 for DefaultExecutionEventBus)
… and improve docs - Add ExecutionEventName type alias for 'event' | 'finished' (ishymko suggestion) - Export ExecutionEventName from server/index.ts - Update JSDoc to clarify subset of EventEmitter methods available - Add @param/@returns tags to public methods for consistency - Make internal Maps readonly - Add helper methods for listener tracking (reduce duplication) - Update section dividers to match codebase style
- Add @edge-runtime/vm for Edge runtime testing - Add vitest.edge.config.ts to run tests in Edge environment - Add test:edge npm script for Edge runtime tests - Update CI workflow to run tests on both Node and Edge runtimes - Remove duplicate comment in on() method (explained in addFinishedListenerInternal) - Remove unused return value from untrackWrappedListener - Update LIFO references to match Node.js EventEmitter docs - Rename test to 'remove one instance at a time' (more accurate)
73053b2 to
019e7ed
Compare
ishymko
left a comment
There was a problem hiding this comment.
Last minors and should be good to go 🚀 , thank you for your contribution!
- Add regression tests for listener throw behavior - Refactor vitest.edge.config to merge default config
|
Thank you @drew-foxall! This patch will be be included in the upcoming 0.3.7 package, likely the week of Dec 15 or Dec 22. |
… ExecutionEventBus)
- Merge upstream/main (v0.3.6) with Edge Runtime support (PR a2aproject#219) - Update README with comprehensive server setup examples - Add documentation for Elysia, itty-router, and Fresh servers - Add authentication examples for all frameworks - Update upstream tracking badges to v0.3.6
🤖 I have created a release *beep* *boop* --- ## [0.3.7](v0.3.6...v0.3.7) (2025-12-17) ### Features * add rest client ([#258](#258)) ([96be3a1](96be3a1)) * remove EventEmitter dependency to support Edge Runtime ([#219](#219)) ([6c76fef](6c76fef)), closes [#218](#218) ### Bug Fixes * export transport agnostic errors from client ([#272](#272)) ([23cd42e](23cd42e)) * pass ServerCallContext to getAuthenticatedExtendedAgentCard for REST ([#274](#274)) ([89b141b](89b141b)), closes [#137](#137) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
This PR addresses issue #218 by replacing the Node.js-specific
EventEmitterwith the web-standardEventTargetAPI, enabling the SDK to run natively on edge runtimes (Cloudflare Workers, Deno, Bun) without requiring Node.js compatibility modes.Changes
Replace EventEmitter with EventTarget
File:
src/server/events/execution_event_bus.tseventsmodule dependencyEventTargetAPI available in all modern runtimeson,off,once,removeAllListeners)ExecutionEventNametype alias for'event' | 'finished'CustomEventpolyfill for Node.js 15-18 (built-in from Node.js 19+)Implementation Details
EventEmitter Behavior Compatibility
The implementation faithfully replicates EventEmitter semantics:
thiscontext - Listeners are invoked withthisbound to the event bus instanceType Safety
isAgentExecutionCustomEvent) for safe runtime narrowing - no unsafe casts'finished'event listeners are invoked with no arguments (usingFunction.prototype.call) matching EventEmitter behaviorreadonlyMapsBenefits
nodejs_compatflag needednpm:shims requiredBreaking Changes
Testing
DefaultExecutionEventBuscovering:on/off/oncefunctionalityonce()auto-cleanup behaviorremoveAllListeners()for specific events and all eventsthiscontext bindingRelated Work
I've implemented and tested these changes in my fork:
Closes #218
Release-As: 0.3.7