Repository navigation
EventTarget in Node - should it expose capabilities like removeAllListeners?聽#33558
Description
Activity
Please see the initial discussion in the PR here #33556 (comment)
Personally I see compelling point both ways and I'm not sure what's the better approach.
Carried my response over to here as the flow suggests. The quotes are from #33556
Note that just exposing these capabilities significantly changes the API surface of EventTarget. For example getting a listener count, event names and who's listening to what event invalidates various guarantees the DOM gives.
Sorry for the intervention but are these guarantees codified anywhere, or, in other words, could theoretically relying on such be ever considered basically some sort of bad habit instead?
Can you help me understand what's the benefit of shipping something called EventTarget that doesn't precisely match the browser's EventTarget?
It may be worthwhile to consider if sticking to a definitely incomplete and asymmetrical, but possibly also crippled and/or irrational implementation for whatever reason is of any actual benefit.
There was some significant (but somewhat inconclusive) discussion on this over at WHATWG as well.
If anything, I'd expect the DOM approach to change.
Actually IIRC some engines, like Blink? already implement stuff like this under the hood, even if not exposing it.For development, this is immensely helpful (and please let me point out that an implementation ("circumvention") is quite easy simply by mutating the EventTarget's prototype before any event listeners are being added.
@strongholdmedia they are codified on the web, I believe. The discussion on WHATWG imo was overwhelmingly in favor of not exposing this API.
The primary security JS offers is in first-run code; pretty much anything that requires prior modification is indefensible and thus not a concern.
The latest solution in the PR:
- Exposes
EventTargetthat is as browser compatible as possible. - Exposes
NodeEventTargetthat is compatible as possible.
I think that approach makes sense although we would still have to answer what flavor the emitters we ship ourselves are.
Reacted by Anna Henningsen, Jordan Harband, Alex Yang and Victor Csiky- Exposes
The plan for
AbortController.signalis that it would be aNodeEventTargetin order to preserve ecosystem compatibility (there are existingAbortControllerimplementations whosesignalimplements subsets of bothEventTargetandEventEmitter)@strongholdmedia they are codified on the web, I believe. The discussion on WHATWG imo was overwhelmingly in favor of not exposing this API.
Indeed, you seemed quite overwhelmed to make it appear so.
Exposes NodeEventTarget that is compatible as possible.
I believe this is an elegant and excellent solution.
Then if enough people gets accustomed to actual usability, perhaps people may (will?) get back to the browser matter.
(I would, however, suggest that you nameNodeEventTargetin some more generic way in case any other implementors would "borrow" it later - which, again to me, seems logical.)@strongholdmedia better naming suggestions welcome :]
The plan for AbortController.signal is that it would be a NodeEventTarget in order to preserve ecosystem compatibility
Can you explain this? I just checked really quickly and I don't think
abort-controller(on npm) exposes these?Ah! I was mistaken about the abort-controller module! There was an impl that I came across that did support both but for the life of me I can't remember the name of it and for some reason had it in my mind that it was abort-controller!
The remove functions in event emitter are absolutely question marks either way. The only ones that I absolutely think we need to have are on/once/addListener. Everything else is optional.
@jasnell p
I think Node.js can be helpful (by providing these) as long as we make sure all our internal methods don't rely on this behavior (and work with the regular AbortController API).
It seems to me that the major issue people have with extending these interfaces is we'd be shipping a different API from the DOM's with the same names.
I think we can:
- Ship (Node)EventTarget internally and (Node)AbortController using it.
- As we add APIs with cancellation support through AbortController - all our APIs should not rely on the additional methods on NodeAbortController/NodeEventTarget to make writing universal code easier.
There is a PR to only land EventTarget and remove NodeEventTarget #33665
If anyone has strong reasons about whether or not Node should land the extensions or not - now is a good time to bring them up :]
Also - it's worth mentioning that landing EventTarget now without the extensions doesn't mean we can't add the extensions later on.
Let's keep
NodeEventTargetfor now but I will open a PR removing theremoveAllListeners()functions.Reacted by Jordan Harband and Benjamin Gruenbaum@strongholdmedia better naming suggestions welcome :]
Well.. If the latest implementation has something along
getEventListeners, it could beTransparentEventTarget. :)If anyone has strong reasons about whether or not Node should land the extensions or not - now is a good time to bring them up :]
After I spent the whole day working with the legacy/browser
EventTargetidea I personally find it frustrating that many people can be quite preoccupied with like introducing or deprecating something with little importance (like removal of the commas in the new CSS "functions"), or sizable nuisance (like the notion ofabusing the optional SameSite attributefor cookies that are set in the very same manner for more than five years now), but try to stick to irrational legacies in questions that might matter more (like this).As most of you surely know, the current legacy
EventTargetimplementation mirrors that of Adobe Flash as in ActionScript 3.
It was dated enough when I got to use it (some 11 years ago) already.
Meanwhile, it is comparable to nothing around - I find even thesetTimeout/clearTimeoutapproach more concise - they are returning handles that can be used for removal, and do that by value, as opposed to the strict reference comparison used here.But then again, please note that I realize that even this approach does not try to solve the problems (if they even are, judged subjectively).
I would just like to pinpoint that sometimes it may be justifiable to observe the proposed evolution from more than a single angle. If anything it is exactly stuff like this that I consider worthwhile updating/extending/enhancing.
Also sorry for the late reply - I mixed up the comments here with those at the PR, and as such, I thought the discussion passed me by long since.
After reviewing the code and finding that there is no means to retrieve the current listeners, I think the
removeAllListenersis justified.@strongholdmedia ActionScript's
EventDispatcher(it didn't have an EventTarget) is very close. For what it's worth it did exposehasEventListener. It also had apriorityparameter, no passive listeners and other differences that are a result of a divergence probably.Although, you might have it backwards, the event model in ActionScript 3 is based on the DOM's and not vice versa:
ActionScript 3.0 introduces a single event-handling model that replaces the many different event-handling mechanisms that existed in previous versions of the language. The new event model is based on the Document Object Model (DOM) Level 3 Events Specification. Although the SWF file format does not adhere specifically to the Document Object Model standard, there are sufficient similarities between the display list and the structure of the DOM to make implementation of the DOM event model possible. An object on the display list is analogous to a node in the DOM hierarchical structure, and the terms display list object and node are used interchangeably throughout this discussion.
FWIW they didn't actually implement the spec which meant code wasn't portable between flash and browser JavaScript and there was a mental overhead regarding differences.
Reacted by Jordan HarbandI think we can close this for now. We have separated out NodeEventTarget and EventTarget and will keep the EventEmitter emulation limited to
NodeEventTargetfor now.Reacted by Jordan Harband
Work is currently underway to add support for EventTarget in Node.js, this was discussed in several summits in the past and is a stepping stone for AbortController and fetch in Node 馃帀
(that PR lands it as experimental and with no open API surface so this issue should not block or delay that PR IMO anyway).
There are (at least) two possible approaches for EventTarget in Node.js:
Note that to my understanding the API does not prohibit us from exposing additional things (like removeAllListeners) but it would still be a violation of the liskov substitution principle (users might expect event listeners to not be removable or discoverable).
Note the APIs are quite different from each other.
The second approach has advantages of probably easier gradual migration and the first approach has the advantage of more closely being universal. Namely: code using the second approach would have to limit itself to not using these APIs in order to be universal in the first place anyway.
Note as prior art,
abort-controllerin NPM supports these methods but browsers do not.cc @jasnell @ljharb @nodejs/events @mcollina