Add EventHelper class with delegation and trigger methods #8126
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive EventHelper utility class that provides jQuery-style event handling APIs using native JavaScript. The implementation includes event namespacing, delegation, and various convenience methods for managing DOM events.
Key changes:
- New EventHelper class with methods for adding, removing, and triggering events with namespace support
- Event delegation functionality for handling dynamically added elements
- Comprehensive test suite with 483 lines covering various scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| src/utils/events.js | Implements the EventHelper class with event handling methods including on, off, trigger, delegate, undelegate, one, hover, and utility methods for managing event listeners |
| tests/utils/events.test.js | Comprehensive test suite covering basic event handling, namespace support, event delegation, custom events, convenience methods, handler management, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Remove all event listeners from an element | ||
| * @param {Element|string} element - DOM element or selector | ||
| * @param {string} [namespace] - Optional namespace to filter by | ||
| * @returns {Element} The element itself |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The element itself", but the method can also return null when the element parameter is null/undefined or when a selector doesn't match any element (line 261). The return type should be documented as {Element|null} to accurately reflect all possible return values.
| * @returns {Element} The element itself | |
| * @returns {Element|null} The element itself or null if no element is found |
| } | ||
|
|
||
| // Parse event type and namespace | ||
| const [eventType, namespace] = event.split('.') |
There was a problem hiding this comment.
Namespace parsing discards multiple namespace segments. When an event string like "click.ns1.ns2" is provided, the split and destructuring at line 27 only captures namespace = "ns1", discarding "ns2". The expected behavior for jQuery-style namespacing is that the entire string after the first dot should be treated as a single namespace. Change the parsing to preserve the full namespace: const parts = event.split('.'); const eventType = parts[0]; const namespace = parts.slice(1).join('.') or use const [eventType, ...nsParts] = event.split('.'); const namespace = nsParts.join('.').
| parent._delegates = parent._delegates.filter(delegate => { | ||
| const matches = | ||
| delegate.selector === selector && | ||
| delegate.eventType === eventType && | ||
| (!namespace || delegate.namespace === namespace) && | ||
| (!handler || delegate.handler === handler) |
There was a problem hiding this comment.
The undelegate method has a potential bug in handler matching. Line 185 checks (!handler || delegate.handler === handler), but delegate.handler is the wrapped delegate handler created in the delegate method, not the original handler passed by the user. This means the optional handler parameter for selective removal will never match, making it impossible to remove a specific delegated handler while keeping others. The comparison should be against the original handler, which would require storing it in the delegate metadata.
| /** | ||
| * Remove event listener with namespace support | ||
| * @param {Element|string} element - DOM element or selector | ||
| * @param {string} event - Event name with optional namespace | ||
| * @param {Function} [handler] - Specific handler to remove (optional) | ||
| * @returns {Element} The element itself |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The element itself", but the method can also return null when the element parameter is null/undefined or when a selector doesn't match any element (line 64). The return type should be documented as {Element|null} to accurately reflect all possible return values.
| * @param {string} selector - Child element selector | ||
| * @param {string} event - Event name with optional namespace | ||
| * @param {Function} [handler] - Specific handler to remove | ||
| * @returns {Element} The parent element |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The parent element", but the method can also return null when the parent parameter is null/undefined or when a selector doesn't match any element (line 174). The return type should be documented as {Element|null} to accurately reflect all possible return values.
| * @returns {Element} The parent element | |
| * @returns {Element|null} The parent element |
| element._eventHandlers[eventType].push({ | ||
| handler, | ||
| namespace, | ||
| wrappedHandler, | ||
| original: handler | ||
| }) |
There was a problem hiding this comment.
The handler is stored twice in the handlers array. Line 44 stores handler while line 47 stores original: handler, which are both references to the same function. This creates redundancy in the stored data structure. Consider removing the handler property and only keeping original.
| if (typeof element === 'string') element = DOMHelper.$(element) | ||
| if (!element || !element._eventHandlers) return element | ||
|
|
||
| const [eventType, namespace] = event.split('.') |
There was a problem hiding this comment.
Namespace parsing discards multiple namespace segments. When an event string like "click.ns1.ns2" is provided, the split and destructuring at line 66 only captures namespace = "ns1", discarding "ns2". This is the same issue as in the on method. Change the parsing to preserve the full namespace: const parts = event.split('.'); const eventType = parts[0]; const namespace = parts.slice(1).join('.').
| /** | ||
| * Trigger custom event | ||
| * @param {Element|string} element - DOM element or selector | ||
| * @param {string} event - Event name | ||
| * @param {*} [detail] - Custom data to pass with event | ||
| * @param {Object} [options] - Event options | ||
| * @returns {Element} The element itself |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The element itself", but the method can also return null when the element parameter is null/undefined or when a selector doesn't match any element (line 111). The return type should be documented as {Element|null} to accurately reflect all possible return values.
| * @param {string} event - Event name with optional namespace | ||
| * @param {Function} handler - Event handler function | ||
| * @param {Object|boolean} [options] - Event options or useCapture flag | ||
| * @returns {Element} The parent element |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The parent element", but the method can also return null when the parent parameter is null/undefined or when a selector doesn't match any element (line 134). The return type should be documented as {Element|null} to accurately reflect all possible return values.
| * @returns {Element} The parent element | |
| * @returns {Element|null} The parent element |
| /** | ||
| * One-time event listener - automatically removed after first trigger | ||
| * @param {Element|string} element - DOM element or selector | ||
| * @param {string} event - Event name with optional namespace | ||
| * @param {Function} handler - Event handler function | ||
| * @param {Object|boolean} [options] - Event options or useCapture flag | ||
| * @returns {Element} The element itself |
There was a problem hiding this comment.
Incomplete JSDoc return type documentation. The return type is documented as "The element itself", but the method can also return null when the element parameter is null/undefined or when a selector doesn't match any element (line 207). The return type should be documented as {Element|null} to accurately reflect all possible return values.
🤔Type of Request
📝Changelog
Add EventHelper class with delegation and trigger methods
☑️Self Check before Merge