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

doc: fix mismatched arguments of `NodeEventTarget` by deokjinkim · Pull Request #45678 · nodejs/node · GitHub

/ node Public

doc: fix mismatched arguments of NodeEventTarget - #45678

Merged
aduh95 merged 2 commits into
nodejs:mainfrom
deokjinkim:221130_revise_nodeeventtarget_doc
Jan 19, 2023
ConversationCommits (2)ChecksFiles changed
Merged

doc: fix mismatched arguments of NodeEventTarget#45678
aduh95 merged 2 commits into
nodejs:mainfrom
deokjinkim:221130_revise_nodeeventtarget_doc

Conversation

deokjinkim commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Arguments of some APIs are mismatched and 2 APIs are not described.

doc,lib: fix mismatched arguments of NodeEventTarget
2c146b6 
Arguments of some APIs are mismatched and 2 APIs are not
described.
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 29, 2022

deokjinkim commented Nov 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

cc @nodejs/documentation

I have some questions about NodeEventTarget.

  1. addListener doesn't have options argument, but corresponding function removeListener has options(capture). So I think options of removeListner is useless. Is current API implementation reasonable?
    (1) addListener :

    node/lib/internal/event_target.js

    Line 881 in 20af3a6

    addListener(type, listener) {

    (2) removeListener :

    node/lib/internal/event_target.js

    Line 857 in 20af3a6

    removeListener(type, listener, options) {

  2. once has { once: true } by default. But test case passes option such as {once: false} to once function. Is this bug?
    (1) once:

    node/lib/internal/event_target.js

    Line 907 in 20af3a6

    once(type, listener) {

    (2) test case: test/parallel/test-nodeeventtarget.js, 113 line

deokjinkim commented Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

cc @nodejs/documentation

I have some questions about NodeEventTarget.

  1. addListener doesn't have options argument, but corresponding function removeListener has options(capture). So I think options of removeListner is useless. Is current API implementation reasonable?
    (1) addListener :

    node/lib/internal/event_target.js

    Line 881 in 20af3a6

    addListener(type, listener) {

    (2) removeListener :

    node/lib/internal/event_target.js

    Line 857 in 20af3a6

    removeListener(type, listener, options) {

  2. once has { once: true } by default. But test case passes option such as {once: false} to once function. Is this bug?
    (1) once:

    node/lib/internal/event_target.js

    Line 907 in 20af3a6

    once(type, listener) {

    (2) test case: test/parallel/test-nodeeventtarget.js, 113 line

@aduh95 I think implementation of NodeEventTarget is different from document. Could you please let me know who is in charge of NodeEventTarget?

aduh95 commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

/cc @nodejs/events

aduh95 reviewed Dec 5, 2022
View reviewed changes
Comment thread doc/api/events.md
Comment on lines +2071 to +2091
#### `nodeEventTarget.setMaxListeners(n)`

<!-- YAML
added: v14.5.0
-->

* `n` {number}

Node.js-specific extension to the `EventTarget` class that sets the number
of max event listeners as `n`.

#### `nodeEventTarget.getMaxListeners()`

<!-- YAML
added: v14.5.0
-->

* Returns: {number}

Node.js-specific extension to the `EventTarget` class that returns the number
of max event listeners.

aduh95 Dec 5, 2022

Copy link
Copy Markdown
Contributor

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

We would need to update this list as well:

node/doc/api/events.md

Lines 1646 to 1650 in 016abb6

2. The `NodeEventTarget` does not emulate the full `EventEmitter` API.
Specifically the `prependListener()`, `prependOnceListener()`,
`rawListeners()`, `setMaxListeners()`, `getMaxListeners()`, and
`errorMonitor` APIs are not emulated. The `'newListener'` and
`'removeListener'` events will also not be emitted.

deokjinkim Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

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

Updated list of unsupported function in NodeEventTarget. Thank you for comment.

aduh95 added doc Issues and PRs related to the documentations. and removed needs-ci PRs that need a full CI run. labels Dec 5, 2022
aduh95 changed the title doc,lib: fix mismatched arguments of NodeEventTarget doc: fix mismatched arguments of NodeEventTarget Dec 5, 2022
lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
nodejs-github-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 Jan 18, 2023

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/45678
✔  Done loading data for nodejs/node/pull/45678
----------------------------------- PR info ------------------------------------
Title      doc: fix mismatched arguments of `NodeEventTarget` (#45678)
Author     Deokjin Kim  (@deokjinkim)
Branch     deokjinkim:221130_revise_nodeeventtarget_doc -> nodejs:main
Labels     doc
Commits    2
 - doc,lib: fix mismatched arguments of `NodeEventTarget`
 - update list of unsupported function in `NodeEventTarget`
Committers 1
 - Deokjin Kim 
PR-URL: https://github.com/nodejs/node/pull/45678
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45678
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 29 Nov 2022 16:38:36 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/45678#pullrequestreview-1213082334
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/45678#pullrequestreview-1223262811
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3952162434

lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 19, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

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

aduh95 merged commit b804399 into nodejs:main Jan 19, 2023

aduh95 commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

Landed in b804399

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
doc: fix mismatched arguments of NodeEventTarget
e36af49 
Arguments of some APIs are mismatched and 2 APIs are not as
described.

PR-URL: #45678
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
doc: fix mismatched arguments of NodeEventTarget
2537d33 
Arguments of some APIs are mismatched and 2 APIs are not as
described.

PR-URL: #45678
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
doc: fix mismatched arguments of NodeEventTarget
de2b7a9 
Arguments of some APIs are mismatched and 2 APIs are not as
described.

PR-URL: #45678
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

aduh95 aduh95 left review comments
jasnell jasnell approved these changes
benjamingr benjamingr approved these changes

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL