Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding opts when removing event listener #408

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

sakin
Copy link
Contributor

@sakin sakin commented Sep 13, 2024

I'm not totally sure if this is the correct fix, but recently was trying to add a capture option when listening to an event on window. If you don't add the option when removing the event listener I don't believe it properly removes the listener.

Please let me know if there's a better way to do this, would love to learn!

Copy link
Owner

@Sub-Xaero Sub-Xaero left a comment

Choose a reason for hiding this comment

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

@sakin This looks perfect!
We already extract any non-standard opts at


so opts will only contain the usual EventListener options
Screenshot 2024-09-14 at 09 52 03
Screenshot 2024-09-14 at 09 52 09

RemoveEventListener only cares about capture, whereas the opts contains the full set of options for AddEventListener, so if we wanted to be safe, we could extract the options that only AddEventListener cares about (i.e. once or passive) before passing opts to RemoveEventListener, but RemoveEventListener will safely ignore any options it doesnt understand, so we can pass it as is.

So, IHMO what you've got is perfect, lets get it merged!

@Sub-Xaero Sub-Xaero merged commit ff39aa1 into Sub-Xaero:main Sep 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants