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

Allow removing event listeners by group #469

Closed
wants to merge 3 commits into from
Closed

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 23, 2017

Closes #208.

Tests: web-platform-tests/wpt#6331

Chrome is tentatively interested in this, although the first step is getting a spec and tests up for review. Would love to hear if other implementers are interested--- @smaug----, @cdumez, @travisleithead?

I decided to go with an overload instead of a new method, so we can leave new method names for something potentially nicer like on/off. Overloading for this pattern is pretty common, e.g. it is what jQuery does. And introducing two similar-but-different method names like removeEventListener + deleteEventListener seemed like a bad idea.

I also added an overload removeEventListener(type, { group }) which is not really necessary. We could get rid of that if we don't like it. But it fell out naturally.

This also preserves what I'm pretty sure is a bug, per #468, but at least it centralizes the probably-buggy check.


Preview | Diff

@domenic domenic added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Jun 23, 2017
domenic added a commit to web-platform-tests/wpt that referenced this pull request Jun 23, 2017
@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Jun 23, 2017
@smaug----
Copy link
Collaborator

( This is a pull request so try to not comment API proposal here. )

dom.bs Outdated
to remove all event listeners in that group:

<pre class=lang-javascript>
oven.addEventListener("turnon", () => { &hellip; }, { group: "cookies" })
Copy link
Member

@Krinkle Krinkle Jun 27, 2017

Choose a reason for hiding this comment

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

cookies is a fun and lightweight example word, but I think in this context it has the risk of confusing less-experienced readers due to being an overloaded term. Perhaps we can come up with another example here, or if nothing else, plain "example".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Will make it brownies instead during the next revision.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jun 30, 2017
@kenchris
Copy link

I was wondering whether supplying a signal like the AbortController/AbortSignal would make more sense instead of groups. Then you can abort multiple things with the same controller.

const controller = new AbortController();
accelerometer.addEventListener('reading', listener, {signal:controller.signal});
controller.abort();

@whatwg whatwg deleted a comment Mar 4, 2019
@annevk
Copy link
Member

annevk commented Dec 8, 2020

I suggest we close this now that you can use abort signals as grouping mechanism thanks to @benjamingr's work. Thoughts?

@domenic
Copy link
Member Author

domenic commented Dec 8, 2020

Oh, right, this was mine originally. Yeah, signals is basically a better version of this; let's close!

@domenic domenic closed this Dec 8, 2020
@domenic domenic deleted the event-namespaces branch December 8, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: events
Development

Successfully merging this pull request may close these issues.

Removing event listeners through an identifier
5 participants