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.

Sign up for GitHub

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

Jump to bottom

[v12.x] events: assume an EventEmitter if emitter.on is a function #35818

Closed
wants to merge 1 commit into from
Closed

[v12.x] events: assume an EventEmitter if emitter.on is a function #35818

wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Oct 26, 2020

Assume that the emitter argument of EventEmitter.once() is an
EventEmitter if emitter.on is a function.

Refs: 4b3654e923e7c3c2
Refs: websockets/ws#1795

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

@lpinca
events: assume an EventEmitter if emitter.on is a function
32067a6 
Assume that the `emitter` argument of `EventEmitter.once()` is an
`EventEmitter` if `emitter.on` is a function.

Refs: nodejs@4b3654e923e7c3c2
Refs: websockets/ws#1795
@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. v12.x labels Oct 26, 2020
@lpinca
Copy link
Member Author

lpinca commented Oct 26, 2020 •
edited
Loading

This fixes a breaking change introduced in 9150c4dc72.

The EventTarget implementation is still experimental so I don't think
#33556 and all the PRs linked in #33556 (comment) will be packported to v12.x

@lpinca lpinca changed the title events: assume an EventEmitter if emitter.on is a function [v12.x] events: assume an EventEmitter if emitter.on is a function Oct 26, 2020
@benjamingr
Copy link
Member

@lpinca note that that PR was added in order to support EventTarget in userland, Node's own EventTarget isn't actually directly exposed to user-land (outside AbortController).

@benjamingr
Copy link
Member

I don't think this use case (of something that behaves like both eventemitter and eventtarget) is common and a quick GitHub search doesn't find interesting results.

I am fine with landing this change as semver-patch.

Preferring EventTarget makes sense to me - the only thing that might break is NodeEventTarget but that looks fine to me.

The biggest difference here is error handling (adding the error event handler to the EventEmitter).

@benjamingr
Copy link
Member

Also fwiw I am fine with this change in master (and not just 12.x)

@lpinca
Copy link
Member Author

lpinca commented Oct 26, 2020 •
edited
Loading

On master, v15.x, and v14.x it's already like this. See linked refs.

@benjamingr
Copy link
Member

@lpinca Ah lmao, I am the author of that and absolutely forgot about that 😅

@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@lpinca lpinca mentioned this pull request Nov 4, 2020
Merged
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34049/

@MylesBorins
Copy link
Contributor

@nodejs/events can we please get one more sign off on this

@MylesBorins
Copy link
Contributor

pinging @nodejs/events can we please get another sign-off here?

maybe @mcollina ?

mcollina
mcollina approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@MylesBorins
Copy link
Contributor

Landed in a211c70e04aa

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@lpinca @MylesBorins
events: assume an EventEmitter if emitter.on is a function
b484732 
Assume that the `emitter` argument of `EventEmitter.once()` is an
`EventEmitter` if `emitter.on` is a function.

Refs: 4b3654e923e7c3c2
Refs: websockets/ws#1795

PR-URL: #35818
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@lpinca lpinca deleted the fix/events-once branch November 16, 2020 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lpinca @benjamingr @nodejs-github-bot @MylesBorins @mcollina