-
Notifications
You must be signed in to change notification settings - Fork 29.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: fix TypeError with EventEmitter#on() abuse #527
Conversation
LGTM |
// https://github.com/iojs/io.js/issues/523 - second call should not throw. | ||
var recv = {}; | ||
EventEmitter.prototype.on.call(recv, 'event', function() {}); | ||
EventEmitter.prototype.on.call(recv, 'event', function() {}); |
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.
Shouldn't this error happen on the first call also without the patch?
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.
No. The code path isn't reached the first time due to an optimization in how listeners are stored.
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.
Huh. Interesting.
Commit 3e1b1dd missed a few files in test/parallel, this commit rectifies that. Only test/parallel/test-url.js still has a copyright header. I left it in because the original author is neither an io.js contributor nor a StrongLoop employee. PR-URL: nodejs#527 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Add missing commas in parallel/test-event-emitter-get-max-listeners. Comma-less style is fine and dandy but it throws off vim's autoindent. PR-URL: nodejs#527 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Commit 2931348 added EventEmitter#getMaxListeners() but introduced a regression when people abuse EventEmitter.prototype.on.call() to call EventEmitter#on() on a non-EE object. Add a workaround for that. Fixes: nodejs#523 PR-URL: nodejs#527 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
LGTM |
141e603
to
ee9cd00
Compare
So nice. thank you. |
Thanks for the fix on behalf of sequelize :). And yes, we are clearly abusing EventEmitter by squashing bits and pieces of it onto promises until we can remove eventemitter support completely. Btw, we are working on getting rid of the offending bit of code, discussion here sequelize/sequelize#2882 |
Commit 2931348 added EventEmitter#getMaxListeners() but introduced a
regression when people abuse EventEmitter.prototype.on.call() to call
EventEmitter#on() on a non-EE object. Add a workaround for that.
Fixes: iojs#523
R=@yosuke-furukawa