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

doc: fix mismatched arguments of NodeEventTarget #45678

Merged
merged 2 commits into from
Jan 19, 2023
Merged

doc: fix mismatched arguments of NodeEventTarget #45678

merged 2 commits into from
Jan 19, 2023

Conversation

deokjinkim
Copy link
Contributor

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

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

deokjinkim commented Nov 30, 2022 •
edited
Loading

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
Copy link
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
Copy link
Contributor

aduh95 commented Dec 5, 2022

/cc @nodejs/events

aduh95
aduh95 reviewed Dec 5, 2022
View reviewed changes
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@aduh95 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 aduh95 changed the title doc,lib: fix mismatched arguments of NodeEventTarget doc: fix mismatched arguments of NodeEventTarget Dec 5, 2022
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot 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
Copy link
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 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 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
Copy link
Collaborator

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

@aduh95 aduh95 merged commit b804399 into nodejs:main Jan 19, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jan 19, 2023

Landed in b804399

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@deokjinkim @RafaelGSS
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@deokjinkim @juanarbol
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@deokjinkim @juanarbol
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell 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
@deokjinkim @aduh95 @nodejs-github-bot @jasnell @benjamingr @lpinca