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

lib/internal/event_target.js

Name bikeshedding - welcome

👍 from me

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

lib/internal/event_target.js

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.

lib/internal/worker/io.js

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

lib/internal/event_target.js

👍 from me

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

lib/internal/event_target.js

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

lib/internal/event_target.js

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

lib/internal/worker/io.js

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

lib/internal/event_target.js

Minor suggestion: use validateString from /internal/validators.

Suggested change

if (typeof type !== '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

lib/internal/worker/io.js Outdated Show resolved Hide resolved

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-actions bot 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-actions bot 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 f8e9bfc Compare 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-actions bot 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-actions bot 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-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2020

github-actions bot 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!

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-actions bot commented Nov 6, 2020

Uh oh!

github-actions bot commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants