events: support emit on nodeeventtarget by benjamingr · Pull Request #35851 · nodejs/node

benjamingr · 2020-10-28T14:25:09Z

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

benjamingr · 2020-10-28T14:25:25Z

Name bikeshedding - welcome

👍 from me

benjamingr · 2020-10-28T14:26:28Z

I am not sure this is the right error to throw and I am not sure throwing is the right behavior.
Other alternatives can be:
Wrap it in an array
Ignore additional arguments and emit the first one
Behave differently for emitter style and target style listeners.

Ignore additional arguments and emit the first one
I think I’d have a mild preference for this one, but really only a mild one.

addaleax · 2020-10-28T14:30:29Z

👍 from me

addaleax · 2020-10-28T14:31:22Z

Ignore additional arguments and emit the first one
I think I’d have a mild preference for this one, but really only a mild one.

addaleax · 2020-10-28T14:31:58Z

NodeEventTarget is purely internal, so I would not mention it here, tbh. Maybe just say `.emit()` on a Node.js `EventTarget`?

benjamingr · 2020-10-29T08:54:23Z

cc @nodejs/workers

nodejs-github-bot · 2020-10-29T08:55:35Z

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

nodejs-github-bot · 2020-10-29T12:23:50Z

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

ZYSzys · 2020-10-29T15:32:36Z

Minor suggestion: use validateString from /internal/validators.
Suggested change
if(typeoftype!=='string'){
validateString(type,'type')

benjamingr · 2020-10-31T09:35:38Z

Accidentially converted to draft - will convert back, mixed this up with the AbortSignal PR 😅

nodejs-github-bot · 2020-10-31T09:43:02Z

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

nodejs-github-bot · 2020-11-02T10:29:19Z

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

nodejs-github-bot · 2020-11-04T10:28:21Z

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

github-actions · 2020-11-06T10:42:02Z

Commit Queue failed

- Loading data for nodejs/node/pull/35851
✔ Done loading data for nodejs/node/pull/35851
----------------------------------- PR info ------------------------------------
Title events: support emit on nodeeventtarget (#35851)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch benjamingr:node-event-target-emit -> nodejs:master
Labels author ready, lts-watch-v14.x, worker
Commits 1
- events: support emit on nodeeventtarget
Committers 1
- Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/35851
Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35851
Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
--------------------------------------------------------------------------------
⚠ Commits were pushed since the last review:
⚠ - events: support emit on nodeeventtarget
✔ Last GitHub Actions successful
ℹ Last Full PR CI on 2020-11-06T10:15:15Z: https://ci.nodejs.org/job/node-test-pull-request/34119/
- Querying data for job/node-test-pull-request/34119/
✔ Build data downloaded
✔ Last Jenkins CI successful
ℹ This PR was created on Wed, 28 Oct 2020 14:25:09 GMT
✔ Approvals: 4
✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35851#pullrequestreview-518726152
✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35851#pullrequestreview-519608507
✔ - Yongsheng Zhang (@ZYSzys): https://github.com/nodejs/node/pull/35851#pullrequestreview-519795411
✔ - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/35851#pullrequestreview-521105281
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/349293015

github-actions · 2020-11-06T10:51:06Z

Landed in 2a1273c...c5477be

PR-URL: #35851 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

PR-URL: nodejs#35851 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

PR-URL: #35851 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

NodeEventTarget.emit() is not described in document. Plus, make type parameter of removeAllListeners as optional. Refs: #35851 PR-URL: #46356 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

NodeEventTarget.emit() is not described in document. Plus, make type parameter of removeAllListeners as optional. Refs: nodejs/node#35851

benjamingr requested a review from addaleax October 28, 2020 14:25

nodejs-github-bot added the worker Issues and PRs related to Worker support. label Oct 28, 2020

benjamingr commented Oct 28, 2020
View reviewed changes

Comment threadlib/internal/worker/io.js Outdated

benjamingr mentioned this pull request Oct 28, 2020
worker_threads parentPort extends EventTarget in Node 14 but extends EventEmitter in Node 12 #35835
Closed

addaleax approved these changes Oct 28, 2020
View reviewed changes

benjamingr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 28, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020

Trott approved these changes Oct 29, 2020
View reviewed changes

ZYSzys approved these changes Oct 29, 2020
View reviewed changes

rickyes approved these changes Oct 31, 2020
View reviewed changes

benjamingr marked this pull request as draft October 31, 2020 09:33

benjamingr marked this pull request as ready for review October 31, 2020 09:35

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2020

addaleax added the lts-watch-v14.x label Nov 2, 2020

events: support emit on nodeeventtarget
f8e9bfc

benjamingr force-pushed the node-event-target-emit branch from 4c1d6a7 to f8e9bfcCompare November 4, 2020 10:21

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2020

github-actionsBot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 6, 2020

addaleax approved these changes Nov 6, 2020
View reviewed changes

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 6, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2020

github-actionsBot closed this Nov 6, 2020

danielleadams mentioned this pull request Nov 9, 2020
v15.2.0 proposal #36055
Merged

targos added backport-open-v14.x and removed lts-watch-v14.x labels Apr 24, 2021

targos mentioned this pull request Apr 24, 2021
[v14.x] Backport AbortController and friends #38386
Closed

targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged

F3n67u mentioned this pull request Jun 21, 2022
events: add CustomEvent#43514
Merged

deokjinkim mentioned this pull request Jan 26, 2023
doc: add emit to NodeEventTarget #46356
Closed

abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
doc: add emit to NodeEventTarget
81a7ffe
NodeEventTarget.emit() is not described in document. Plus, make type parameter of removeAllListeners as optional. Refs: nodejs/node#35851

Uh oh!

Uh oh!

Conversation

benjamingr commented Oct 28, 2020

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamingr commented Oct 29, 2020

Uh oh!

nodejs-github-bot commented Oct 29, 2020

Uh oh!

nodejs-github-bot commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Oct 31, 2020

Uh oh!

nodejs-github-bot commented Oct 31, 2020

Uh oh!

nodejs-github-bot commented Nov 2, 2020

Uh oh!

nodejs-github-bot commented Nov 4, 2020

Uh oh!

github-actionsBot commented Nov 6, 2020

Uh oh!

github-actionsBot commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants