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.

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

Update events.md #33513

Closed
wants to merge 2 commits into from
Closed

Update events.md #33513

wants to merge 2 commits into from

Conversation

shawsourav
Copy link
Contributor

@shawsourav shawsourav commented May 22, 2020

Since the section refers to EventEmitter, instances in the example should be created of the same class EventEmitter.

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

Since the section refers to EventEmitter, instances in the example should be created of the same class EventEmitter.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels May 22, 2020
@benjamingr
Copy link
Member

The top of the page has a (for the example):

class MyEmitter extends EventEmitter {}

So it looks like the current behavior is intentional. I'm +0 on changing it fwiw :]

doc/api/events.md Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

BridgeAR commented May 23, 2020

@benjamingr I had another look and the code block where MyEmitter is originally defined is too far off from this documentation part. It does not really seem to be related anymore. Thus, I think this change is sound.

@BridgeAR
Copy link
Member

Ping @shawsourav

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 16, 2020

Landed in 9e5a27a

aduh95 pushed a commit that referenced this pull request Oct 16, 2020
Since the section refers to EventEmitter, instances in the example
should be created of the same class EventEmitter.

PR-URL: #33513
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 closed this Oct 16, 2020
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Since the section refers to EventEmitter, instances in the example
should be created of the same class EventEmitter.

PR-URL: #33513
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants