Skip to content

Conversation

@pertrai1
Copy link
Owner

@pertrai1 pertrai1 commented Jan 13, 2026

Summary by CodeRabbit

  • New Features

    • Added EventEmitter utility implementing observer/publish-subscribe pattern with registration, unregistration, and event emission capabilities.
  • Documentation

    • Added comprehensive documentation for the new EventEmitter utility with usage examples.
    • Updated progress tracker (6/75 problems completed).
  • Tests

    • Added extensive test suite covering core functionality, edge cases, and integration scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@pertrai1 pertrai1 requested a review from Copilot January 13, 2026 14:24
@pertrai1 pertrai1 added the code challenge this is a coding challenge label Jan 13, 2026
@continue
Copy link

continue bot commented Jan 13, 2026

All Green - Keep your PRs mergeable

Learn more

All Green is an AI agent that automatically:

✅ Addresses code review comments

✅ Fixes failing CI checks

✅ Resolves merge conflicts


Unsubscribe from All Green comments

@github-actions
Copy link

@codex review

1 similar comment
@github-actions
Copy link

@codex review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

A new EventEmitter utility has been introduced to GreatFrontEnd with comprehensive documentation and test coverage. The implementation provides observer/pub-sub pattern functionality, and progress trackers have been updated across README files to reflect the completion.

Changes

Cohort / File(s) Summary
Documentation Updates
README.md, greatfrontend/gfe-75/README.md, greatfrontend/gfe-75/javascript-utilities/README.md
Updated progress counts from 10→11 (main README) and 5→6 (gfe-75 README). Added EventEmitter entry to completed problems list.
EventEmitter Documentation
greatfrontend/gfe-75/javascript-utilities/event-emitter/README.md
New documentation describing the EventEmitter utility, observer pattern, and usage examples with on(), emit(), and off() methods.
EventEmitter Implementation
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
New TypeScript utility implementing IEventEmitter interface with private Map-based listener management. Includes on() for registration, off() for removal, and emit() for invocation with boolean return indicating listener existence. Clones listener array during emission to safely handle runtime modifications.
EventEmitter Test Suite
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
Comprehensive test coverage (367 lines) spanning listener registration/removal, multi-argument emission, listener ordering, error propagation, runtime addition/removal during emission, pub-sub patterns, and interface compliance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hop, hop! A new event-emitter hops along,
With listeners gathering in harmonious song,
On, off, and emit—the trio so keen,
The finest pub-sub ever seen!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'GFE Custom EventEmitter' accurately reflects the main change: implementing a new EventEmitter utility for the GreatFrontEnd problem set, with complete test coverage and documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch gfe-1132025

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Jan 13, 2026

Claude finished @pertrai1's task in 1m 41s —— View job


GreatFrontEnd EventEmitter Review ✅

Excellent implementation! This is a clean, well-tested EventEmitter that correctly implements the observer pattern. The code demonstrates strong understanding of JavaScript fundamentals and includes comprehensive test coverage.


1. Correctness ✅

Strengths:

  • All core functionality works correctly (on, off, emit)
  • Handles edge cases properly (non-existent events, multiple registrations, removal during emit)
  • The .slice() on line 44 is a critical detail that prevents issues when listeners modify the listener array during iteration

Edge Cases Handled:

  • ✅ Removing listeners during emit (test on line 253)
  • ✅ Adding listeners during emit (test on line 273)
  • ✅ Multiple registrations of same listener (test on line 55)
  • ✅ Removing non-existent listeners (test on line 105)

2. API Design & Usability ✅

Excellent choices:

  • Method chaining support (return this) - very ergonomic
  • Clear, intuitive method names matching Node.js EventEmitter
  • Proper TypeScript interface for type safety
  • Boolean return from emit() indicating if listeners were called

Minor improvements suggested:

  • See inline comment on line 2: Consider type EventListener = (...args: any[]) => void instead of Function
  • See inline comment on line 11: Add cleanup for empty event arrays to prevent memory accumulation

3. Memory Management & Performance

Strong points:

  • Private field #events prevents external tampering ✅
  • Line 44's .slice() prevents modification issues during emit ✅
  • Reference equality in off() works correctly ✅

Optimization opportunities:

  • Line 27: Use indexOf() instead of findIndex() for better performance (see inline comment)
  • Consider cleaning up empty event arrays after all listeners are removed (see inline comment on line 11)

Time Complexity:

  • on(): O(1) - array push
  • off(): O(n) - findIndex + splice, where n = number of listeners for that event
  • emit(): O(n) - iterate all listeners + slice copy

Space Complexity: O(E × L) where E = unique events, L = average listeners per event


4. Code Quality ✅

JavaScript Best Practices:

  • ✅ Uses const appropriately
  • ✅ Private fields with # syntax (modern ES2022)
  • ✅ Strict equality (===)
  • ✅ Proper use of Map for event storage
  • ✅ Spread operator for rest parameters

Clean code:

  • Short, focused methods
  • Clear variable names
  • Good separation of concerns

5. Test Coverage 🌟

Outstanding test suite! 367 lines of comprehensive tests covering:

  • Basic functionality (on/off/emit)
  • Method chaining
  • Multiple listeners and events
  • Edge cases (listeners added/removed during emit)
  • Error handling (line 239)
  • Integration scenarios (pub-sub pattern, event-driven workflows)
  • Performance (rapid successive events)

The test comments (e.g., lines 250, 264, 282) demonstrate clear understanding of the expected behavior.


🎓 Learning Summary

Pattern: Observer (Pub-Sub)

This is a classic Observer Pattern implementation, also called Publish-Subscribe. It decouples event emitters from event listeners, allowing one-to-many communication.

Key Insight 🔑

The critical insight is using .slice() on line 44 to create a snapshot of the listeners array before iteration. This prevents bugs when listeners modify the array during emit (adding/removing listeners). Without this, you'd have:

  • Iterator invalidation if listeners are removed mid-loop
  • Unexpected behavior if new listeners are added (they might be called immediately or skipped)

This is a common pattern in event systems and is essential for robust implementations.

JavaScript Concepts Demonstrated

  1. Private Fields (#events) - ES2022 syntax for true privacy
  2. Method Chaining - Fluent API design by returning this
  3. Reference Equality - Using === to identify specific listener functions
  4. Function Context - Using apply() to control this binding (line 46)
  5. Rest Parameters - ...args for variable argument lists (line 37)
  6. Map Data Structure - Efficient key-value storage for events

Common Pitfalls Avoided

  1. Forgot to copy array before iteration - You correctly used .slice()
  2. Memory leaks from unbounded growth - Though you could improve with cleanup (see inline comment)
  3. Incorrect this context - Properly used apply()

Related Problems to Practice

  1. Event Emitter with once() - Add a method that registers a listener that automatically removes itself after first call
  2. Event Emitter with Priorities - Allow listeners to specify execution priority/order
  3. Async Event Emitter - Modify to support async listeners and await all listeners in emit()
  4. Custom Observable/RxJS basics - Extend this to support operators like map, filter, debounce

Real-World Applications

  • Node.js EventEmitter (HTTP servers, streams, custom events)
  • DOM events (addEventListener/removeEventListener)
  • Redux/MobX state management libraries
  • WebSocket/real-time communication patterns
  • React event handling (synthetic events)

Alternative Approaches

  1. WeakMap for listeners: Could use WeakMap if listeners are always objects (allows garbage collection)
  2. Set instead of Array: Would prevent duplicate registrations automatically, but changes semantics
  3. Trie for event namespacing: For events like user.login.success, could use a tree structure

Trade-offs: Your array-based approach is simple, matches Node.js behavior (allows duplicates), and performs well for typical use cases.


Summary

This is a strong, production-ready implementation with excellent test coverage. The code is clean, follows best practices, and demonstrates solid understanding of JavaScript fundamentals and design patterns.

Minor improvements suggested in inline comments are optional optimizations - the current implementation is already very good!

Grade: A 🎉


@chatgpt-codex-connector
Copy link

To use Codex here, create a Codex account and connect to github.

1 similar comment
@chatgpt-codex-connector
Copy link

To use Codex here, create a Codex account and connect to github.

@github-actions
Copy link

github-actions bot commented Jan 13, 2026

🧮 Solution Analysis

PR: GFE Custom EventEmitter
Files Changed: greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts

📊 Code Quality Results

ESLint Analysis

  • Errors: 0
  • Warnings: 2

Issues by file:

  • /home/runner/work/coding-challenges/coding-challenges/greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts: 0 errors, 2 warnings

🔍 Complexity Analysis

event-emitter

📋 Analysis Results:

🔍 Automated Complexity Analysis

⚠️ Disclaimer: This is an automated analysis that may not be 100% accurate.
Always verify the complexity analysis manually, especially for complex algorithms.
Dynamic Programming, recursive, and mathematical algorithms may need manual review.

File: greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
Generated: 2026-01-13T14:24:50.801Z

Time Complexity

Estimated: O(1)
Description: Constant time - single operations
Confidence: 52.0%

⚠️ Low Confidence: Please manually verify this analysis.
💡 Tip: Add // @complexity: O(n) time, O(1) space to override automated detection.

Space Complexity

Estimated: O(n)
Reason: Additional data structures

Data Structures Used

  • Array: []
    • access: O(1)
    • search: O(n)
    • insertion: O(1) amortized
    • deletion: O(n)
  • Hash Map: new Map()
    • access: O(1) average
    • search: O(1) average
    • insertion: O(1) average
    • deletion: O(1) average
  • Hash Map: Map(
    • access: O(1) average
    • search: O(1) average
    • insertion: O(1) average
    • deletion: O(1) average
  • Stack/Queue: .push(
    • push/pop: O(1)
    • shift/unshift: O(n) for arrays

Code Metrics

  • Lines of Code: 45
  • Cyclomatic Complexity: 5
  • Max Nesting Level: 3

Recommendations

  • VERIFICATION: ⚠️ Manual Review Needed: Low confidence score suggests manual verification is required

event-emitter.test

📋 Analysis Results:

🔍 Automated Complexity Analysis

⚠️ Disclaimer: This is an automated analysis that may not be 100% accurate.
Always verify the complexity analysis manually, especially for complex algorithms.
Dynamic Programming, recursive, and mathematical algorithms may need manual review.

File: greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
Generated: 2026-01-13T14:24:50.752Z

Time Complexity

Estimated: O(1)
Description: Constant time - single operations
Confidence: 50.3%

⚠️ Low Confidence: Please manually verify this analysis.
💡 Tip: Add // @complexity: O(n) time, O(1) space to override automated detection.

Space Complexity

Estimated: O(1)
Reason: Constant extra space

Data Structures Used

  • Array: [1, 2, 3]
    • access: O(1)
    • search: O(n)
    • insertion: O(1) amortized
    • deletion: O(n)
  • Stack/Queue: .push(
    • push/pop: O(1)
    • shift/unshift: O(n) for arrays

Code Metrics

  • Lines of Code: 273
  • Cyclomatic Complexity: 6
  • Max Nesting Level: 5

Recommendations

  • VERIFICATION: ⚠️ Manual Review Needed: Low confidence score suggests manual verification is required
  • READABILITY: Deep nesting detected. Consider early returns or helper functions

💡 Analysis Tips

  • Time Complexity: Focus on the dominant operation in loops
  • Space Complexity: Consider auxiliary data structures
  • Optimization: Look for opportunities to improve efficiency
  • Edge Cases: Ensure your solution handles empty inputs, null/undefined, edge conditions

📁 Analysis Files Created

Detailed analysis files have been automatically generated and committed to docs/analysis/ for future reference.

🎯 Next Steps

  1. Review any ESLint errors/warnings above
  2. Consider the complexity analysis - is this the optimal approach?
  3. Test with edge cases mentioned in the problem description
  4. Add comments explaining complex logic
  5. Check the docs/analysis/ directory for detailed complexity analysis files

Analysis generated for PR files only

@github-actions
Copy link

✅ Test Results

Tests have been executed for this pull request.

📊 Coverage Summary

Metric Coverage
Statements 94.36%
Branches 88.88%
Functions 85.36%
Lines 94.42%

Check the workflow logs for detailed test output.

@github-actions
Copy link


🤖 Gemini Code Review (Model: gemini-2.5-flash)

Here's a detailed review of your EventEmitter implementation:


1. Pattern Identification

This solution implements the Observer Pattern (also known as the Publish-Subscribe pattern). In this pattern:

  • Subjects (Publishers) maintain a list of their dependents, called Observers (Subscribers), and notify them of any state changes, usually by calling one of their methods.
  • In this case, the EventEmitter class acts as the Subject, and the listener functions are the Observers. Events are the "state changes" that trigger notifications.

2. Complexity Verification

Let N_e be the number of listeners registered for a specific eventName.
Let L be the total number of listeners across all events.
Let E be the number of unique event names.

  • on(eventName, listener)

    • Stated Time Complexity: Not explicitly stated, but typically O(1) average.
    • Correct Time Complexity: O(1) average.
      • Map.has() and Map.set() operations are O(1) on average.
      • Array.push() is O(1).
    • Stated Space Complexity: Not explicitly stated.
    • Correct Space Complexity: O(1) for a single call (stores one function reference). Overall, the EventEmitter stores O(L) space for all listeners.
  • off(eventName, listener)

    • Stated Time Complexity: Not explicitly stated.
    • Correct Time Complexity: O(N_e) average and worst case.
      • Map.has() and Map.get() are O(1) average.
      • Array.findIndex() iterates through the listeners array, taking O(N_e) time in the worst case (listener is at the end or not found).
      • Array.splice() to remove an element in the middle of an array takes O(N_e) time as subsequent elements need to be shifted.
    • Stated Space Complexity: Not explicitly stated.
    • Correct Space Complexity: O(1) additional space.
  • emit(eventName, ...args)

    • Stated Time Complexity: Not explicitly stated.
    • Correct Time Complexity: O(N_e) average and worst case.
      • Map.has() and Map.get() are O(1) average.
      • Array.slice() creates a shallow copy of the listeners array, which takes O(N_e) time.
      • Array.forEach() iterates through the copied listeners array, calling each listener. This takes O(N_e) iterations. The time complexity of executing each listener itself depends on its internal logic, but the overhead of calling them is constant.
    • Stated Space Complexity: Not explicitly stated.
    • Correct Space Complexity: O(N_e) for the shallow copy of the listeners array created by slice().

3. Key Insight

The key insight for a robust EventEmitter (especially emit) is to iterate over a copy of the registered listeners when an event is emitted.

  • Why it's crucial: If you were to iterate directly over the original array of listeners, and one of the listeners being invoked decided to call emitter.on() or emitter.off() for the same event during the emit cycle, it could lead to unpredictable behavior:
    • Skipped listeners: If a listener before the current one is removed, the indices shift, potentially causing the next listener to be skipped.
    • Infinite loops: If a listener re-adds itself or another listener in a way that affects the loop's bounds.
    • Out-of-bounds errors: If listeners are removed and the loop counter exceeds the new array length.

By creating const listeners = this.#events.get(eventName).slice();, you ensure that the emit operation works with a snapshot of the listeners at the moment emit was called, providing stability and predictable behavior regardless of modifications made by the listeners themselves.

4. Edge Case Analysis

The current solution handles most common edge cases correctly:

  • No listeners registered for an event: emit correctly returns false.
  • Removing a non-existent listener: off gracefully handles this by returning this without error.
  • Removing a listener from a non-existent event: off also handles this gracefully.
  • Adding the same listener multiple times: on allows this, and emit calls it multiple times, matching standard EventEmitter behavior. off removes only one instance per call.
  • Listeners modifying the listener list during emit: Handled correctly due to slice().
  • Empty event names: The Map can correctly use empty strings as keys, so on('', listener) and emit('') would work as expected.
  • Null/undefined listeners: While vi.fn() prevents this in tests, if a null or undefined function was explicitly passed as a listener, listener.apply() would throw an error. A production-grade EventEmitter might validate listener is a function.
  • Listeners throwing errors: As shown in your tests, if a listener throws an error, the emit call itself will throw, and subsequent listeners in that specific emit cycle for that event will not be called. This is a valid design choice (often desired for immediate feedback), but some implementations might catch and log errors, or emit a special 'error' event to prevent crashing the entire emit process. Your current implementation's behavior is consistent with Node.js's EventEmitter for non-'error' events.

5. Learning Points

  • Similar problems using this pattern:
    • UI Event Systems: The browser's DOM events (addEventListener, removeEventListener) are a prime example. Custom UI components often expose their own events.
    • State Management Libraries: Simplified versions of Redux or Pub-Sub stores often use this pattern to notify subscribers when state changes.
    • Backend Microservices: Inter-service communication often uses message queues, which conceptually rely on a publish-subscribe model.
    • Custom Observable Implementations: Building your own basic Observable class.
  • Common mistakes people make with this pattern:
    • Modifying the listener list during iteration: As discussed in Key Insight, this is the most common and subtle bug. Using slice() or iterating backwards are common solutions.
    • Memory Leaks: Not properly unsubscribing (off) from events, especially in long-lived applications or single-page applications where components might be unmounted but still subscribed, leading to stale closures and memory retention.
    • Inefficient listener lookup/removal: Using inefficient data structures (e.g., iterating a large array for off when a Map or Set could be faster for unique listeners).
    • Incorrect this context: Not correctly binding the this context when listeners are invoked (your listener.apply(this, args) correctly handles this).
  • Variations of this problem:
    • once(eventName, listener): A method to register a listener that is automatically removed after being called once.
    • prependListener(eventName, listener): Adds a listener to the beginning of the listener array, so it's called before others.
    • removeAllListeners(eventName?): Removes all listeners for a specific event, or all listeners for all events if no eventName is provided.
    • Error Event Handling: Node.js EventEmitter has special behavior for the 'error' event: if it's emitted and no listeners are registered for it, it throws an unhandled error.
    • Wildcard Events: Allowing listeners to subscribe to events matching a pattern (e.g., emitter.on('user:*', handler)).

6. Code Quality

  • Variable Naming:
    • #events is a clear and good choice for the private map.
    • In the off method, the callback parameter listenerIdx is a bit confusing. It's actually the currentListener being checked. Renaming it to l or currentListener would improve clarity:
      const index = listeners.findIndex((l: Function) => l === listener);
  • Code Structure: The class structure is clean, and methods are well-encapsulated.
  • Readability: The code is very readable. TypeScript types (IEventEmitter, Function, Array<any>) significantly enhance clarity.
  • this Context: listener.apply(this, args) correctly sets the this context for the invoked listeners to be the EventEmitter instance, which is standard behavior.

7. Alternative Approaches

  1. Using Map<string, Set<Function>> for Listeners:

    • How it works: Instead of an array Function[] for each event, you could store a Set<Function>.
    • Trade-offs:
      • on: Set.add() is O(1) average. However, it only allows unique listeners. Your current implementation and tests (should allow the same listener to be registered multiple times) indicate that duplicate listeners should be allowed and called multiple times. Using a Set would break this requirement.
      • off: Set.delete() is O(1) average. This would be a significant performance improvement over findIndex + splice (O(N_e)) for removing listeners, if duplicate listeners were not allowed.
      • emit: You would need to convert the Set to an array (Array.from(set)) or iterate using set.forEach(), still resulting in O(N_e) for iteration.
      • Conclusion: While Set offers faster off operations, it fundamentally changes the behavior regarding duplicate listeners, making it unsuitable if multiple identical subscriptions are a requirement.
  2. Using a Map<string, { id: number, fn: Function }[]> with Unique IDs:

    • How it works: Instead of storing functions directly, store objects { id: number, fn: Function } in the array. on would assign a unique ID, off would remove by ID.
    • Trade-offs:
      • on: O(1) to add. Requires a mechanism to generate unique IDs.
      • off: Still requires findIndex (O(N_e)) to find the object by function reference, then splice (O(N_e)). No real performance gain over the current approach for off.
      • emit: Same as current, O(N_e).
      • Conclusion: Adds complexity without significant performance benefits for this specific problem where off needs to remove by function reference. It might be useful if listeners were identified by IDs instead of function references.

Your chosen approach using Map<string, Function[]> is robust and aligns well with the common requirements and behavior of EventEmitter implementations, especially with the critical slice() for emit. The only area for potential optimization would be off, but it's often a less frequent operation than emit or on, making the current compromise acceptable for clarity and feature parity.

@github-actions
Copy link


🤖 Gemini Code Review (Model: gemini-2.5-flash)

Here's a review of the EventEmitter solution, structured to provide educational feedback.

1. Pattern Identification

This solution implements the Observer Pattern (also known as the Publish-Subscribe pattern).

  • Publisher/Subject: The EventEmitter instance itself.
  • Subscribers/Observers: The listener functions.
  • Event: The eventName string.

The core idea is to establish a one-to-many dependency between objects so that when one object changes state, all its dependents are notified and updated automatically. In this case, when an event is "published" (emit), all "subscribers" (on registered listeners) for that event are "notified" (executed).

2. Complexity Verification

Time Complexity

  • on(eventName, listener):

    • Stated: Not explicitly stated, but typically considered O(1).
    • Correct: O(1) on average.
    • Reasoning: Map.has, Map.get, Map.set, and Array.push operations all take constant time on average. In the worst case (e.g., hash collisions in a Map or very large arrays requiring reallocation), these could degrade, but for practical purposes, they are considered O(1).
  • off(eventName, listener):

    • Stated: Not explicitly stated.
    • Correct: O(L) where L is the number of listeners currently registered for eventName.
    • Reasoning: Map.has and Map.get are O(1) average. However, Array.prototype.findIndex() iterates through the listeners array, taking O(L) time in the worst case (listener is at the end or not found). Array.prototype.splice() also takes O(L) time in the worst case as it may need to shift subsequent elements in the array.
  • emit(eventName, ...args):

    • Stated: Not explicitly stated.
    • Correct: O(L) where L is the number of listeners currently registered for eventName.
    • Reasoning: Map.has and Map.get are O(1) average. Array.prototype.slice() creates a shallow copy of the listeners array, which takes O(L) time. The subsequent forEach loop then iterates through L listeners, calling each one. Each listener execution is O(1) in terms of the emit method itself (its internal complexity depends on the listener's function).

Space Complexity

  • Stated: Not explicitly stated.
  • Correct: O(E + L)
  • Reasoning:
    • E represents the number of unique event names that have at least one listener. The Map stores E keys (event names).
    • L represents the total number of listener functions stored across all event types. Each listener function (and the reference to it) consumes memory. The Map stores E arrays, and these arrays collectively store L function references.
    • The emit method temporarily creates a shallow copy of the listeners array using slice(). This adds O(L) temporary space during the emit call, but this is garbage collected after the method finishes.

3. Key Insight

The key insight for building a robust EventEmitter lies in two main aspects:

  1. Centralized Registry: Maintaining a central data structure (like a Map or an object) that maps each eventName to a collection of its registered listener functions. This allows efficient lookup of which functions to call when a specific event is emitted.
  2. Snapshot for Emission: When emit is called, it's crucial to iterate over a copy (a snapshot) of the listeners array for that event. This prevents issues if a listener function itself adds or removes other listeners for the same event during the ongoing emission cycle. If the original array were mutated during iteration, it could lead to skipped listeners, infinite loops, or other unpredictable behavior. The use of this.#events.get(eventName).slice() correctly implements this snapshot behavior.

4. Edge Case Analysis

The current solution handles many edge cases well, but here are some specific considerations:

  • Empty eventName: The Map can store empty strings as keys, so emitter.on('', listener) would work.
  • listener is not a function: The current implementation allows registering anything as a listener. If emitter.on('test', 'not a function') is called, then emitter.emit('test') would later try to execute 'not a function'.apply(...), leading to a TypeError.
    • Suggestion: Add a type check in on to ensure listener is a function: if (typeof listener !== 'function') { throw new TypeError('Listener must be a function'); }.
  • Removing a non-existent listener or from a non-existent event: Handled correctly by off returning this without error.
  • Listeners added/removed during emit: Handled correctly due to slice() creating a snapshot of listeners at the beginning of the emit call. Listeners added during the current emit cycle will not be called in that cycle, but will be in subsequent ones. Listeners removed during the current emit cycle will still be called in that cycle (because they were in the snapshot), but not in subsequent ones.
  • Listener throws an error: The current implementation (using forEach) will stop the emit process immediately if a listener throws an uncaught error. Subsequent listeners for that event will not be called. The provided tests confirm this behavior. This is a valid design choice, but some EventEmitter implementations (like Node.js') have special handling for 'error' events or allow for custom error handling to ensure all listeners are called if possible.

5. Learning Points

  • Similar problems/patterns:
    • Custom Event System: Building a custom event system for a UI component or a part of a larger application.
    • State Management Libraries: Many state management patterns (e.g., Redux, Vuex, even simple observable patterns) leverage the observer pattern to notify subscribers of state changes.
    • Browser EventTarget: The native DOM event system (addEventListener, removeEventListener, dispatchEvent) is a prime example of the observer pattern.
    • Backend Pub/Sub: Message queues and broker systems (e.g., Kafka, RabbitMQ, Redis Pub/Sub) are distributed implementations of the publish-subscribe pattern.
  • Common mistakes people make with this pattern:
    • Modifying listener array during iteration: As mentioned in Key Insight, iterating directly over the mutable listener array can lead to bugs. Using slice() to create a copy is crucial.
    • Memory Leaks: If listeners are registered (on) but never unregistered (off), they can prevent objects they close over from being garbage collected, leading to memory leaks, especially in long-running applications or single-page applications with many component lifecycles.
    • Incorrect this context: Forgetting to bind this or using apply/call can lead to listeners being executed with the wrong this context. The current solution uses listener.apply(this, args) which correctly sets this to the EventEmitter instance, which is standard.
  • Variations of this problem:
    • once(eventName, listener): A listener that is automatically removed after being invoked once.
    • removeAllListeners(eventName?): A method to remove all listeners for a specific event, or all listeners for all events if eventName is omitted.
    • listenerCount(eventName): Returns the number of listeners for a given event.
    • prependListener(eventName, listener): Adds a listener to the beginning of the list, so it's called before other listeners.
    • Asynchronous emit: Emit events asynchronously (e.g., using setTimeout or queueMicrotask) to decouple listener execution from the emitter.
    • Error Handling: Implement specific error handling for listeners that throw, perhaps by emitting a special 'error' event or providing a global error handler.

6. Code Quality

  • Variable Naming:

    • #events is a clear and appropriate private field name.
    • listenerIdx in the off method's findIndex callback is slightly ambiguous. While listenerIdx === listener clarifies its intent, currentListener or simply l might be marginally clearer for the iterated element. However, it's generally readable.
  • Code Structure: The class structure with public methods and a private field is excellent and follows modern JavaScript best practices. The methods are concise and focused on their single responsibility.

  • Readability: The code is very readable. Using Map for event storage is clear.

  • TypeScript:

    • Using Function as a type is generally discouraged in TypeScript as it's too broad. A more specific type like (...args: any[]) => void would be better if the listener signature is unknown, or even more specific if the expected arguments are known for certain events. The IEventEmitter interface is a good addition for explicit typing.
    • Consider adding readonly to #events in the constructor to emphasize that the Map instance itself isn't reassigned, only its contents are mutated.
    export class EventEmitter implements IEventEmitter {
      readonly #events: Map<string, Function[]>; // More specific type for #events
    
      constructor() {
        this.#events = new Map<string, Function[]>();
      }
    
      on(eventName: string, listener: (...args: any[]) => void): IEventEmitter { // More specific listener type
        if (typeof listener !== 'function') {
          throw new TypeError('Listener must be a function');
        }
        if (!this.#events.has(eventName)) {
          this.#events.set(eventName, []);
        }
        this.#events.get(eventName)!.push(listener); // Use non-null assertion as we just checked/set
        return this;
      }
    
      off(eventName: string, listener: (...args: any[]) => void): IEventEmitter { // More specific listener type
        const listeners = this.#events.get(eventName);
        if (!listeners) { // Check if listeners array exists
          return this;
        }
        const index = listeners.indexOf(listener); // Use indexOf for direct comparison
        if (index > -1) { // Check if listener was found
          listeners.splice(index, 1);
        }
        return this;
      }
    
      emit(eventName: string, ...args: any[]): boolean {
        const listeners = this.#events.get(eventName);
        if (!listeners || listeners.length === 0) {
          return false;
        }
        // Iterate over a copy to prevent issues if listeners are added/removed during emit
        const listenersToCall = listeners.slice();
        listenersToCall.forEach((l: (...args: any[]) => void) => { // More specific listener type
          l.apply(this, args);
        });
        return true;
      }
    }
    • off improvement: listeners.indexOf(listener) is generally more idiomatic and potentially more efficient than findIndex if you just need to find the first occurrence of the exact function reference, as it avoids creating an anonymous function for the predicate.

7. Alternative Approaches

  1. Using a plain JavaScript object instead of Map:

    • Approach: Store event names as keys and arrays of listeners as values in a plain object (this.events = {}).
    • Trade-offs:
      • Pros: Slightly simpler syntax for access (this.events[eventName]).
      • Cons: Object keys are always converted to strings, which is usually fine for event names but Map handles non-string keys robustly. Map generally offers better performance for frequent additions/deletions, especially with a large number of varying keys, and is designed for this use case. Map also preserves insertion order of keys (though not strictly necessary here).
  2. Using Set<Function> instead of Array<Function> for listeners:

    • Approach: Instead of storing an array of listeners for each event, store a Set of listeners (Map<string, Set<Function>>).
    • Trade-offs:
      • Pros: Set automatically enforces uniqueness of listeners. If you call on with the same listener multiple times, it will only be registered once. This simplifies on (set.add(listener)) and off (set.delete(listener)) to O(1) average time complexity.
      • Cons: The problem description and tests imply that duplicate listeners should be allowed (e.g., emitter.on('test', listener); emitter.on('test', listener); emitter.emit('test'); expect(listener).toHaveBeenCalledTimes(2);). If this is a strict requirement, then Set would not be suitable, and an Array is necessary. If uniqueness is desired, Set is a more elegant solution. The emit method would iterate over set.values().

@@ -0,0 +1,50 @@
export interface IEventEmitter {
on(eventName: string, listener: Function): IEventEmitter;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type Safety Improvement

Consider using a more specific type than Function for better type safety:

type EventListener = (...args: any[]) => void;

This makes it clear that listeners don't return values (or their return values are ignored), which matches the actual behavior in emit() on line 45-46.

#events;

constructor() {
this.#events = new Map();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory Consideration

The Map stores arrays of listeners indefinitely. Consider adding a method to clean up empty event arrays to prevent memory accumulation:

private cleanupEvent(eventName: string): void {
  if (this.#events.has(eventName) && this.#events.get(eventName).length === 0) {
    this.#events.delete(eventName);
  }
}

Then call this in off() after removing a listener. This prevents the Map from growing unbounded with event names that no longer have listeners.

return this;
}
const listeners = this.#events.get(eventName);
const index = listeners.findIndex(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization Opportunity

findIndex() with a comparison function is unnecessary here. Since you're checking reference equality, use indexOf() directly:

const index = listeners.indexOf(listener);

This is simpler, more performant, and achieves the same result.

}
const listeners = this.#events.get(eventName).slice();
listeners.forEach((listener: Function) => {
listener.apply(this, args);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this Context Consideration

Using listener.apply(this, args) binds this to the EventEmitter instance. While this matches Node.js behavior, it's worth considering whether this is the desired behavior for your use case.

Alternative approaches:

  1. Call directly (loses this context): listener(...args)
  2. Bind at registration (lets users control context): Store bound functions in on()
  3. Current approach (EventEmitter as this): Matches Node.js

The current implementation is correct for mimicking Node.js EventEmitter. Just be aware that listeners receive the EventEmitter as this, which might be unexpected for some users.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a custom EventEmitter implementation to the GreatFrontEnd collection. The EventEmitter follows the observer/publish-subscribe pattern, similar to Node.js's EventEmitter, allowing registration of event listeners, removal of listeners, and event emission with arguments.

Changes:

  • Implemented EventEmitter class with on(), off(), and emit() methods using TypeScript
  • Added comprehensive test suite with 367 lines covering edge cases, method chaining, concurrent modifications, and integration scenarios
  • Updated progress trackers across README files (6/75 GFE problems, 11 total frontend problems)

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts Core EventEmitter implementation using Map for event storage and private fields for encapsulation
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts Comprehensive test suite covering all methods, edge cases, and integration scenarios
greatfrontend/gfe-75/javascript-utilities/event-emitter/README.md Problem description with usage examples demonstrating the pub-sub pattern
greatfrontend/gfe-75/javascript-utilities/README.md Added EventEmitter to the completed problems list
greatfrontend/gfe-75/README.md Updated progress counter from 5 to 6 completed problems
README.md Updated GreatFrontEnd problem count from 10 to 11

}
const listeners = this.#events.get(eventName);
const index = listeners.findIndex(
(listenerIdx: Function) => listenerIdx === listener
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name listenerIdx is misleading because it doesn't represent an index - it represents a listener function. Consider renaming it to something like listenerFn or l to better reflect its purpose.

Suggested change
(listenerIdx: Function) => listenerIdx === listener
(listenerFn: Function) => listenerFn === listener

Copilot uses AI. Check for mistakes.
}

export class EventEmitter implements IEventEmitter {
#events;
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The private field #events lacks a type annotation. While TypeScript can infer the type, explicit typing improves code clarity and maintainability. Consider adding the type annotation: #events: Map<string, Function[]>;

Suggested change
#events;
#events: Map<string, Function[]>;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
@greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts:
- Around line 239-251: The test name is misleading and missing an assertion for
listener2: rename the test to reflect that errors from listeners propagate
(e.g., "should propagate errors from a listener and prevent subsequent
listeners") and keep the existing expect(() =>
emitter.emit('test')).toThrow('Listener error'); then add an assertion verifying
listener2 was not called (expect(listener2).not.toHaveBeenCalled()); reference
EventEmitter, emit, on, listener1 and listener2 to locate where to change.
🧹 Nitpick comments (4)
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts (3)

8-8: Add explicit type annotation to the private field.

Per coding guidelines, TypeScript should use explicit type annotations. The #events field lacks a type annotation.

Suggested fix
-  #events;
+  #events: Map<string, Function[]>;

14-20: Add explicit return type annotation to on method.

Per coding guidelines, use explicit type annotations for function return types.

Suggested fix
-  on(eventName: string, listener: Function) {
+  on(eventName: string, listener: Function): IEventEmitter {

22-35: Add explicit return type annotation to off method.

Per coding guidelines, use explicit type annotations for function return types.

Suggested fix
-  off(eventName: string, listener: Function) {
+  off(eventName: string, listener: Function): IEventEmitter {
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts (1)

1-4: Consider adding edge case tests for event name validation.

The test suite is comprehensive, but consider adding tests for edge cases around event names such as empty strings, special characters, or whitespace-only names to verify the implementation handles these gracefully. Based on learnings, edge case tests for empty inputs and boundary conditions are recommended.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80c2d85 and c90c8ed.

📒 Files selected for processing (6)
  • README.md
  • greatfrontend/gfe-75/README.md
  • greatfrontend/gfe-75/javascript-utilities/README.md
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/README.md
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/README.md

📄 CodeRabbit inference engine (CLAUDE.md)

Include problem title and difficulty badge, link to the problem on the respective platform (LeetCode, GreatFrontEnd, etc.), problem description, examples and constraints

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/README.md
  • README.md
  • greatfrontend/gfe-75/javascript-utilities/README.md
  • greatfrontend/gfe-75/README.md
README.md

📄 CodeRabbit inference engine (AGENTS.md)

Update README.md when adding new solutions

Files:

  • README.md
**/*.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.ts: Use explicit type annotations for function parameters and return types
Follow strict TypeScript configuration (defined in tsconfig.json)
Target ES2020 with CommonJS modules
Strict mode enabled with ES2020 target and CommonJS modules for TypeScript

**/*.ts: Use explicit type annotations for function parameters and return types in TypeScript solutions
Follow strict TypeScript configuration with ES2020 target and CommonJS modules
Avoid any type unless absolutely necessary in TypeScript solutions
Use interfaces for object types in TypeScript solutions

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*.{ts,js}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,js}: Test complex algorithms with helper functions, data structure implementations, solutions with multiple edge cases, GreatFrontEnd API implementations, and reusable utility functions
Use descriptive variable names (single letters like i, j, k are acceptable for loop iterators and standard mathematical contexts)
Don't use deprecated JavaScript features

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*-*.{ts,js,md}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use kebab-case for file names matching problem names

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*.{js,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{js,ts}: Verify the solution correctly solves the problem for all test cases, including edge cases: empty arrays, single elements, negative numbers, null values
Document and verify time and space complexity using Big O notation, typically in code comments or docs/analysis/ files
Use clear, descriptive variable names (not just i, j, k unless in simple loops) and add comments for non-obvious logic or algorithm steps
Use const by default, let only when reassignment is needed; avoid var except for LeetCode solution function definitions
Use strict equality (===) over loose equality (==)
Use modern ES6+ syntax (arrow functions, destructuring, spread operators) where appropriate and prefer built-in methods (.map(), .filter(), .reduce())

**/*.{js,ts}: Use const by default, let only when reassignment is needed in JavaScript/TypeScript
Use strict equality (===) over loose equality (==) in JavaScript/TypeScript
Use descriptive variable names; single letters like i, j, k acceptable only for standard loop contexts
Prefer built-in array methods (.map(), .filter(), .reduce()) in JavaScript/TypeScript
Fix ESLint errors with npm run quality:fix and ensure proper code formatting with Prettier

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/greatfrontend/**/*.{js,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

**/greatfrontend/**/*.{js,ts}: Verify correct usage of browser APIs (DOM, Fetch, Storage, etc.), check for browser compatibility considerations, and validate event handling patterns with proper cleanup
Check for unnecessary re-renders or DOM manipulations, verify efficient data structure usage for UI updates, and look for debouncing/throttling where appropriate
Verify parameter validation, error handling, and consider edge cases specific to browser environments (empty inputs, invalid types)

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts
  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*.test.{ts,js}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.test.ts or *.test.js naming convention for test files

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*.test.{js,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

Use Vitest for testing complex solutions, data structures, and GreatFrontEnd implementations; ensure tests handle edge cases, async operations, and cleanup logic

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.test.ts: Test all examples from problem description in test files
Add edge case tests: empty inputs, single elements, nulls, boundary conditions in test files
Use descriptive test names that explain what is being tested in test files
Group related tests using describe blocks in test files
Include both positive and negative test cases in test files
Add test cases for new solutions and update tests when refactoring existing solutions

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
**/*-*.test.ts

📄 CodeRabbit inference engine (AGENTS.md)

Test files should use kebab-case naming with .test.ts extension: {problem-name}.test.ts

Files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
🧠 Learnings (7)
📚 Learning: 2025-12-30T13:41:58.622Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T13:41:58.622Z
Learning: Applies to leetcode/**/README.md : Keep README files up to date with problem counts and organization

Applied to files:

  • README.md
📚 Learning: 2025-12-30T13:41:24.644Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T13:41:24.644Z
Learning: Applies to **/README.md : Include problem title and difficulty badge, link to the problem on the respective platform (LeetCode, GreatFrontEnd, etc.), problem description, examples and constraints

Applied to files:

  • README.md
📚 Learning: 2025-12-30T13:41:58.622Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T13:41:58.622Z
Learning: Applies to **/*.test.ts : Add test cases for new solutions and update tests when refactoring existing solutions

Applied to files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
📚 Learning: 2025-12-30T13:41:58.622Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T13:41:58.622Z
Learning: Applies to **/*.test.ts : Add edge case tests: empty inputs, single elements, nulls, boundary conditions in test files

Applied to files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
📚 Learning: 2025-12-30T13:41:07.657Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T13:41:07.657Z
Learning: Applies to **/*.{ts,js} : Test complex algorithms with helper functions, data structure implementations, solutions with multiple edge cases, GreatFrontEnd API implementations, and reusable utility functions

Applied to files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
  • greatfrontend/gfe-75/README.md
📚 Learning: 2025-12-30T13:41:24.644Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T13:41:24.644Z
Learning: Applies to **/*.test.{js,ts} : Use Vitest for testing complex solutions, data structures, and GreatFrontEnd implementations; ensure tests handle edge cases, async operations, and cleanup logic

Applied to files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
📚 Learning: 2025-12-30T13:41:58.622Z
Learnt from: CR
Repo: pertrai1/coding-challenges PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T13:41:58.622Z
Learning: Applies to **/*.test.ts : Group related tests using `describe` blocks in test files

Applied to files:

  • greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
🧬 Code graph analysis (1)
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts (1)
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts (2)
  • EventEmitter (7-50)
  • IEventEmitter (1-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Agent
  • GitHub Check: review
🔇 Additional comments (9)
greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.ts (1)

37-49: Good defensive cloning of listeners array before iteration.

The use of .slice() on line 44 correctly prevents issues when listeners modify the listener list during emission (e.g., calling off within a listener callback). This is a solid implementation detail.

README.md (1)

10-10: Progress update looks correct.

The GreatFrontEnd problem count increment to 11 aligns with the addition of the EventEmitter solution.

greatfrontend/gfe-75/javascript-utilities/README.md (1)

13-13: EventEmitter entry added correctly.

The new entry follows the established format and links to the appropriate directory.

greatfrontend/gfe-75/javascript-utilities/event-emitter/README.md (1)

1-27: Well-structured documentation with clear examples.

The README provides a concise explanation of the observer pattern, links to Node.js documentation for reference, and includes practical usage examples demonstrating on, emit, and off functionality.

greatfrontend/gfe-75/README.md (1)

7-7: Progress tracker and completed problems list updated correctly.

The status increment to 6/75 and the new EventEmitter entry are consistent with the solution being added.

Also applies to: 17-17

greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts (4)

5-65: LGTM!

The on() test suite is comprehensive, covering basic registration, multiple listeners, per-event separation, method chaining, and non-idempotent duplicate registration. Test names are descriptive and assertions are appropriate.


67-144: LGTM!

The off() test suite thoroughly covers listener removal scenarios including partial removal of duplicate registrations, graceful handling of non-existent listeners/events, and event isolation. The tests correctly verify that only the first occurrence is removed when a listener is registered multiple times.


253-290: LGTM!

These tests correctly verify the copy-on-emit behavior where listeners added or removed during emission don't affect the current emit cycle. The follow-up assertions confirm the changes take effect on subsequent emits.


293-366: LGTM!

The integration tests effectively demonstrate real-world usage patterns including pub-sub messaging, chained event workflows, interface compliance, and basic stress testing with rapid successive events.

Comment on lines +239 to +251
it('should not fail if a listener throws an error', () => {
const emitter = new EventEmitter();
const listener1 = vi.fn(() => {
throw new Error('Listener error');
});
const listener2 = vi.fn();

emitter.on('test', listener1);
emitter.on('test', listener2);

expect(() => emitter.emit('test')).toThrow('Listener error');
// Note: listener2 won't be called because listener1 throws
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Test name is misleading and missing assertion for listener2.

The test is named "should not fail if a listener throws an error" but it actually expects the error to propagate (line 249). Additionally, the comment on line 250 states that listener2 won't be called, but there's no assertion to verify this behavior.

Consider renaming and adding the missing assertion:

Proposed fix
-    it('should not fail if a listener throws an error', () => {
+    it('should propagate errors thrown by listeners', () => {
       const emitter = new EventEmitter();
       const listener1 = vi.fn(() => {
         throw new Error('Listener error');
       });
       const listener2 = vi.fn();

       emitter.on('test', listener1);
       emitter.on('test', listener2);

       expect(() => emitter.emit('test')).toThrow('Listener error');
-      // Note: listener2 won't be called because listener1 throws
+      // listener2 won't be called because listener1 throws first
+      expect(listener2).not.toHaveBeenCalled();
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should not fail if a listener throws an error', () => {
const emitter = new EventEmitter();
const listener1 = vi.fn(() => {
throw new Error('Listener error');
});
const listener2 = vi.fn();
emitter.on('test', listener1);
emitter.on('test', listener2);
expect(() => emitter.emit('test')).toThrow('Listener error');
// Note: listener2 won't be called because listener1 throws
});
it('should propagate errors thrown by listeners', () => {
const emitter = new EventEmitter();
const listener1 = vi.fn(() => {
throw new Error('Listener error');
});
const listener2 = vi.fn();
emitter.on('test', listener1);
emitter.on('test', listener2);
expect(() => emitter.emit('test')).toThrow('Listener error');
// listener2 won't be called because listener1 throws first
expect(listener2).not.toHaveBeenCalled();
});
🤖 Prompt for AI Agents
In
@greatfrontend/gfe-75/javascript-utilities/event-emitter/event-emitter.test.ts
around lines 239 - 251, The test name is misleading and missing an assertion for
listener2: rename the test to reflect that errors from listeners propagate
(e.g., "should propagate errors from a listener and prevent subsequent
listeners") and keep the existing expect(() =>
emitter.emit('test')).toThrow('Listener error'); then add an assertion verifying
listener2 was not called (expect(listener2).not.toHaveBeenCalled()); reference
EventEmitter, emit, on, listener1 and listener2 to locate where to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code challenge this is a coding challenge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants