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

events: remove NodeEventTarget #33665

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 30, 2020

The extending of EventTarget with EventEmitter emulation is contentious
and not something that is strictly necessary for minimal support.

/cc @benjamingr

Signed-off-by: James M Snell jasnell@gmail.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

The extending of EventTarget with EventEmitter emulation is contentious
and not something that is strictly necessary for minimal support.

Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Changes LGTM though I do see value in exposing this like we expose Buffer on top of types arrays.

Where did this get pushback?

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I do not think this is comparable to Buffer, because Buffer existed in Node.js before typed arrays. EventTarget is a new API and nothing in the ecosystem can rely on NodeEventTarget yet because it was never released.

@addaleax
Copy link
Member

Fwiw, I’m somewhat under the impression that this would make turning MessagePorts into actual EventTargets easier – because right now, MessagePorts do implement the EventEmitter API, but only a relatively small subset of the EventTarget API.

Or, put another way: It would require something like NodeEventTarget, whether that’s exposed as a public API or not.

(There’s also the question of whether it’s possible to perform that change to MessagePort without non-trivial performance impact, and whether we’d want to do it at all if the performance impact turns out to be significant.)

@jasnell
Copy link
Member Author

jasnell commented Jun 1, 2020

We can hold off on this if there is other places we can use it. The pushback that I've seen in really in the places we'd use it... like AbortSignal ... e.g. can we extend that interface and still call it AbortSignal? For now, I'll close this PR but I'll open another that removes the removeAllListeners from it

@jasnell jasnell closed this Jun 1, 2020
@benjamingr
Copy link
Member

e.g. can we extend that interface and still call it AbortSignal?

I like the approach we took with NodeEventTarget and we can take a similar approach for AbortController - fwiw there are cases in the DOM That extend AbortController already (like the TaskController API for postTask.

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.

5 participants