fix: issue 1709 eventemitter once#1712
Conversation
The emit() method only cleared the once-handler Set but left the Map entry in place. Use delete() to remove the Map entry entirely, and fire handlers from the disconnected Set reference to prevent re-entrancy issues when a once handler triggers another emit for the same event.
Three changes to EventEmitter: 1. emit() now snapshots and removes once handlers from storage _before_ executing any handler. This prevents re-entrant emit() calls from the same event from firing once handlers prematurely. 2. off() removes empty Set entries from the internal Maps so that hasListeners() and memory usage stay clean after all handlers for an event are removed. 3. Added tests for re-entrancy, late-registration, and Map cleanup to lock in the new behavior. Closes Karanjot786#1709
github-actions
Bot
added
area:core
📝 WalkthroughWalkthrough
ChangesEventEmitter once/off correctness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/events/EventEmitter.ts (1)
35-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsubscribe closure bypasses
off()cleanup logic.The
once()return value directly deletes from the Set without removing empty Map entries, whileon()returns() => this.off(event, handler)which triggers cleanup. If users call the unsubscribe function before the event fires, an empty Set remains in_onceHandlers.For consistency with the cleanup fix in
off():Proposed fix
return () => { - this._onceHandlers.get(event)?.delete(handler); + this.off(event, handler); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/events/EventEmitter.ts` around lines 35 - 37, The unsubscribe closure in the once() method directly deletes from the _onceHandlers Set without removing empty Map entries, causing orphaned empty Sets to remain. To maintain consistency with the cleanup logic in off() and match the pattern used in on(), replace the direct Set deletion with a call to this.off(event, handler) instead. This ensures that when a handler is removed, the corresponding Set is deleted from the Map if it becomes empty.
🧹 Nitpick comments (1)
packages/core/src/events/EventEmitter.test.ts (1)
214-226: 💤 Low valueRemove unnecessary type assertion.
'message'is already a validkeyof TestEvents, so theas anycast is unnecessary. If it's needed to satisfy TypeScript with private property access, add a comment explaining why per coding guidelines.- expect(emitter['_handlers'].has('message' as any)).toBe(false); + expect(emitter['_handlers'].has('message')).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/events/EventEmitter.test.ts` around lines 214 - 226, In the test 'off removes empty Map entries for regular handlers', remove the unnecessary type assertion `as any` from the `'message'` key when accessing the private `_handlers` property with `emitter['_handlers'].has('message' as any)`. The string 'message' is already a valid keyof TestEvents, so the cast is redundant. If TypeScript still requires a type assertion to access the private property, add a comment explaining the necessity per coding guidelines rather than using an implicit `as any` cast.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/events/EventEmitter.ts`:
- Around line 35-37: The unsubscribe closure in the once() method directly
deletes from the _onceHandlers Set without removing empty Map entries, causing
orphaned empty Sets to remain. To maintain consistency with the cleanup logic in
off() and match the pattern used in on(), replace the direct Set deletion with a
call to this.off(event, handler) instead. This ensures that when a handler is
removed, the corresponding Set is deleted from the Map if it becomes empty.
---
Nitpick comments:
In `@packages/core/src/events/EventEmitter.test.ts`:
- Around line 214-226: In the test 'off removes empty Map entries for regular
handlers', remove the unnecessary type assertion `as any` from the `'message'`
key when accessing the private `_handlers` property with
`emitter['_handlers'].has('message' as any)`. The string 'message' is already a
valid keyof TestEvents, so the cast is redundant. If TypeScript still requires a
type assertion to access the private property, add a comment explaining the
necessity per coding guidelines rather than using an implicit `as any` cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6907745-acae-4fb5-90b7-f56d51e966f1
📒 Files selected for processing (2)
packages/core/src/events/EventEmitter.test.tspackages/core/src/events/EventEmitter.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/events/EventEmitter.ts (1)
36-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsubscribe function from
once()doesn't clean up empty Map entries.The
off()method now properly deletes empty Set entries from_onceHandlers, but the unsubscribe function returned byonce()still leaves stale entries. This meanshasListeners()will return incorrect results when users unsubscribe via the returned function rather than callingoff().Consider delegating to
off()or applying the same cleanup:Proposed fix
return () => { - this._onceHandlers.get(event)?.delete(handler); + const set = this._onceHandlers.get(event); + if (set) { + set.delete(handler); + if (set.size === 0) { + this._onceHandlers.delete(event); + } + } };Or simply delegate:
return () => { - this._onceHandlers.get(event)?.delete(handler); + this.off(event, handler); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/events/EventEmitter.ts` around lines 36 - 38, The unsubscribe function returned by the once() method removes the handler from the _onceHandlers Set but does not clean up the Map entry when the Set becomes empty, leaving stale entries that cause hasListeners() to return incorrect results. Fix this by either delegating to the existing off() method (passing the event and handler) instead of directly deleting from the Set, or by adding the same cleanup logic that off() uses to check if the Set is empty and delete the Map entry if so.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/events/EventEmitter.ts`:
- Around line 36-38: The unsubscribe function returned by the once() method
removes the handler from the _onceHandlers Set but does not clean up the Map
entry when the Set becomes empty, leaving stale entries that cause
hasListeners() to return incorrect results. Fix this by either delegating to the
existing off() method (passing the event and handler) instead of directly
deleting from the Set, or by adding the same cleanup logic that off() uses to
check if the Set is empty and delete the Map entry if so.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a99c723-d6a8-4218-a730-62afbc38630e
📒 Files selected for processing (1)
packages/core/src/events/EventEmitter.ts
|
@Karanjot786 Please review this |
1 participant
Description
EventEmitter.once()handlers have two issues:emit()re-entrantly for the same event, the inneremit()fires once handlers prematurely before the outer emit's once phaseoff()leaves emptySetentries in the internal Maps, causinghasListeners()to return stale resultsChanges
EventEmitter.ts
emit(): Snapshots and removes once handlers from the Map before any regular handler executes. Re-entrantemit()calls on the same event won't find the once handlers — they fire at the correct time in the outer call.off(): Deletes emptySetentries from both_handlersand_onceHandlersMaps, keeping internal state clean.EventEmitter.test.ts
off()removes empty Map entries for regular handlersoff()removes empty Map entries for once handlersemit()clears OnceHandler Map entry sohasListeners()returns falseTesting
All 19 tests pass, including 5 new edge-case tests for re-entrancy, late registration, and Map cleanup.
Closes #1709
Summary by CodeRabbit