-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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
not blocking, it's a comment, it can always happen after this lands
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments.
doc/api/events.md
Outdated
args.forEach((arg) => { | ||
parameters += `${arg}, `; | ||
} | ||
); |
There was a problem hiding this comment.
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(', ');
doc/api/events.md
Outdated
// [ | ||
// [Function: firstListener], | ||
// [Function: secondListener], | ||
// [Function: thirdListener] |
There was a problem hiding this comment.
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.
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/.) |
PR-URL: nodejs#28374 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #28374 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes