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

doc: add example about emitter.emit in events documentation #28374

Closed
wants to merge 8 commits into from

Conversation

felipedc09
Copy link
Contributor

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

@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 Jun 21, 2019
doc/api/events.md Outdated Show resolved Hide resolved
@jasnell jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jun 23, 2019
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @vsemozhetbyt's nit fixed

Trott
Trott previously requested changes Jun 25, 2019
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Welcome @felipedc09 and thanks for the pull request. If the purpose is to provide a straightforward and understandable example using emitter.event(), can we simplify this? I'm not sure we need three different events and a class too. Might it not be better to keep it simple along these lines?

const  { EventEmitter } = require('events');
const emitter = new EventEmitter();
emitter.on('event', (arg1, arg) => { console.log('args', arg1, arg2); });
emitter.emit('event', 'arg 1', 'arg 2');
// Prints: args arg 1 arg 2

@Trott Trott dismissed their stale review June 25, 2019 13:55

not blocking, it's a comment, it can always happen after this lands

@felipedc09
Copy link
Contributor Author

Sure @Trott i like your comments, I will change the class but the emitter.emit is using to run synchronously the listeners associated to event, so I want explain the execution of these listeners respect their creation. thanks.

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

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

args.forEach((arg) => {
parameters += `${arg}, `;
}
);
Copy link
Member

Choose a reason for hiding this comment

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

Please move this closing bracket to the line above. It's otherwise a bit confusing.

It could also be simplified by using join:

  const parameters = args.join(', ');

// [
// [Function: firstListener],
// [Function: secondListener],
// [Function: thirdListener]
Copy link
Member

Choose a reason for hiding this comment

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

Super tiny nit: the indentation seems off. To keep it aligned with the starting bracket, it needs another whitespace.

@Trott
Copy link
Member

Trott commented Jul 30, 2019

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jul 31, 2019

Landed in 698d479.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Jul 31, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 31, 2019
PR-URL: nodejs#28374
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Aug 2, 2019
PR-URL: #28374
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. 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.

7 participants