Ensure listener-collection passes options to removeEventListener
#910
Conversation
It's necessary to pass the same `options` to `removeEventListener` as `addEventListener`, or the listener will not be removed.
Codecov Report
@@ Coverage Diff @@
## main #910 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 60 60
Lines 747 747
Branches 271 271
=========================================
Hits 747 747
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
robertknight
left a comment
robertknight
left a comment
There was a problem hiding this comment.
LGTM. Once our browser support target passes Safari >= 15 we can simplify this code by using the signal option as a way to disconnect listeners, instead of having to remember all the addEventListener arguments and pass them to removeEventListener.
robertknight
left a comment
robertknight
left a comment
There was a problem hiding this comment.
LGTM. As noted in Slack, you could get away with abbreviations for the generics if you wanted to make the signature shorter, but I think this is readable as-is.
| eventType: Type, | ||
| listener: (event: EventType<ListenerTarget, Type>) => void, | ||
| options?: AddEventListenerOptions | ||
| ) { |
There was a problem hiding this comment.
It might be helpful for readers to indicate explicitly that the return type is a symbol.
I ran into a confusion today while converting some hooks over to more consistently use
listener-collection. Some worked, but a focus-event-related hook did not. I realized it was becauselistener-collectionwasn't passing theoptionsparameter (when provided) toremoveEventListener. This matters if you have an event that was registered with options (specificallycapture: true) as they don't get removed if you don't pass them options!Converted it to TS while I was in there.
The tests added to the tests module will fail without the change made to the module.