FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v12.x] events: assume an EventEmitter if emitter.on is a function by lpinca · Pull Request #35818 · nodejs/node · GitHub

/ node Public

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

Closed
lpinca wants to merge 1 commit into
nodejs:v12.x-stagingfrom
lpinca:fix/events-once
ConversationCommits (1)ChecksFiles changed
Closed

[v12.x] events: assume an EventEmitter if emitter.on is a function#35818
lpinca wants to merge 1 commit into
nodejs:v12.x-stagingfrom
lpinca:fix/events-once

Conversation

lpinca commented Oct 26, 2020

Copy link
Copy Markdown
Member

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

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 added events Issues and PRs related to the events subsystem / EventEmitter. v12.x labels Oct 26, 2020

lpinca commented Oct 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 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 commented Oct 26, 2020

Copy link
Copy Markdown
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 commented Oct 26, 2020

Copy link
Copy Markdown
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 commented Oct 26, 2020

Copy link
Copy Markdown
Member

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

lpinca commented Oct 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

benjamingr commented Oct 26, 2020

Copy link
Copy Markdown
Member

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

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

This comment has been minimized.

Sign in to view
lpinca mentioned this pull request Nov 4, 2020
Merged

nodejs-github-bot commented Nov 4, 2020

Copy link
Copy Markdown
Collaborator

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

MylesBorins commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

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

maybe @mcollina ?

mcollina approved these changes Nov 16, 2020
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

MylesBorins commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

Landed in a211c70e04aa

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
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 deleted the fix/events-once branch November 16, 2020 19:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
benjamingr benjamingr 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

Back | FazBrowse Home | New Git URL