-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
lib: bind functions on EventEmitter #27427
Conversation
0672e7a
to
88cfac6
Compare
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.
A comparative benchmark run for the events
benchmarks would probably be desirable.
assert.strictEqual(E.off, E.removeListener); | ||
|
||
E.on = function() {}; | ||
assert.strictEqual(E.on, E.addListener); |
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.
Why would this ever be the desired behavior? It breaks the principle of least surprise.
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.
.on
is just an alias to .addListener
. This does prevent some issues where someone wants to change the functionality but forgets to update all functions involved. I guess in this specific case it's fine therefore?
close this pull request, because I think this may break the coding style of some people |
Further modifications from #27325
bind some functions on
EventEmitter
so that we no need to change two functions at the same time.for example:
before
now only need to code one line
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes