emmiter implementation#321
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on building the MyEventEmitter class! The structure is clean and most of the methods are implemented correctly. However, a couple of adjustments are needed to fully meet the project requirements.
I am requesting changes for the following reasons:
removeAllListenersis incomplete: The current implementation only handles removing listeners for a specific event. According to the requirements, if noeventis provided, it should remove all listeners for all events. Please add logic to handle this case.- Incorrect argument handling in
oncemethods: The wrapper functions foronceandprependOnceListeneronly pass the first argument to the original listener. To ensure all data fromemitis correctly forwarded, you should use rest parameters (...args) in the wrapper function and spread syntax (...args) when calling the listener.
Once these two issues are addressed, your solution will be complete. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const onceListener = (data) => { | ||
| listener(data); |
There was a problem hiding this comment.
This implementation of onceListener only captures the first argument passed from emit. To correctly forward all arguments, you should use rest parameters (...args) for the function definition and spread syntax (...args) when calling the original listener.
| const onceListener = (data) => { | ||
| listener(data); |
There was a problem hiding this comment.
Similar to the once method, this onceListener only passes the first argument to the original listener. It should be updated to handle multiple arguments using rest and spread syntax to ensure all data from emit is passed through.
| removeAllListeners(event) { | ||
| this.listeners = this.listeners.filter((l) => l.event !== event); |
There was a problem hiding this comment.
The requirement specifies that if no event is provided, all listeners for all events should be removed. This implementation only handles the case where an event is specified. Consider adding a condition to check if event is undefined, and if so, clear the entire listeners array.
- Modify `once` and `prependOnceListener` methods to accept multiple arguments. - Ensure `removeAllListeners` correctly filters listeners based on the event.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this task, and thank you for addressing the previous feedback! I am requesting a couple more adjustments to align your implementation perfectly with Node.js's EventEmitter behavior. First, the off method should only remove the first matching listener, not all of them; using findIndex and splice is a good way to handle this. Second, the emit method can behave unexpectedly when a listener is removed during the loop (like with once). Iterating over a copy of the listeners array, created with this.listeners.slice(), will solve this issue. After these fixes, your solution will be solid!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| this.listeners = this.listeners.filter( | ||
| (l) => l.event !== event || l.listener !== listener, | ||
| ); |
There was a problem hiding this comment.
The filter method removes all elements that satisfy the condition. However, the standard EventEmitter.off() method should only remove the first matching listener if multiple identical listeners are registered for the same event.
Consider using a method like findIndex to locate the first match and then removing only that single element from the array, for example with splice.
| ); | ||
| } | ||
| emit(event, ...data) { | ||
| this.listeners.forEach((l) => { |
There was a problem hiding this comment.
When a listener is removed from this.listeners during this forEach loop (e.g., a once listener calling off), it can cause unexpected behavior, such as skipping the next listener in the original array if the array is modified in place.
To ensure all listeners registered at the time of emit are called, it's safer to iterate over a copy of the listeners array. You can create a shallow copy by calling this.listeners.slice().forEach(...).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed all the previous feedback, resulting in a robust and correct MyEventEmitter class. Your fixes for the off method to only remove the first matching listener and for the emit method to safely iterate over a copy of the listeners are perfect. The code now meets all task requirements, and I'm happy to approve your solution. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.