-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
events: remove reaches into _events internals #17440
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -574,6 +574,39 @@ to indicate an unlimited number of listeners. | |
|
||
Returns a reference to the `EventEmitter`, so that calls can be chained. | ||
|
||
### emitter.rawListeners(eventName) | ||
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
- `eventName` {any} | ||
|
||
Returns a copy of the array of listeners for the event named `eventName`, | ||
including any wrappers (such as those created by `.once`). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The doc does not explain what a wrapper is in this context, and how it is wrapped. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any thoughts on how we should document this or whether we even should? I'm not certain we want to necessarily encourage the usage. If someone needs it they'll know and otherwise we might want it to be a bit obscure, haha? Not sure... If anyone has any thoughts, please chime in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's documented, it needs to be documented in full. Otherwise it doesn't. I'm fine either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have precedent for including a publicly exposed function that's not documented? Just trying to figure out the direction we would best go here. This isn't something we want to encourage being used but it's a step better than the current situation of reaching into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather this get documented fully. |
||
|
||
```js | ||
const emitter = new EventEmitter(); | ||
emitter.once('log', () => console.log('log once')); | ||
|
||
// Returns a new Array with a function `onceWrapper` which has a property | ||
// `listener` which contains the original listener bound above | ||
const listeners = emitter.rawListeners('log'); | ||
const logFnWrapper = listeners[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add an example also with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. Let me know if this isn't what you had in mind. |
||
|
||
// logs "log once" to the console and does not unbind the `once` event | ||
logFnWrapper.listener(); | ||
|
||
// logs "log once" to the console and removes the listener | ||
logFnWrapper(); | ||
|
||
emitter.on('log', () => console.log('log persistently')); | ||
// will return a new Array with a single function bound by `on` above | ||
const newListeners = emitter.rawListeners('log'); | ||
|
||
// logs "log persistently" twice | ||
newListeners[0](); | ||
emitter.emit('log'); | ||
``` | ||
|
||
[`--trace-warnings`]: cli.html#cli_trace_warnings | ||
[`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners | ||
[`domain`]: domain.html | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ module.exports = EventEmitter; | |
EventEmitter.EventEmitter = EventEmitter; | ||
|
||
EventEmitter.prototype._events = undefined; | ||
EventEmitter.prototype._eventsCount = 0; | ||
EventEmitter.prototype._maxListeners = undefined; | ||
|
||
// By default EventEmitters will print a warning if more than 10 listeners are | ||
|
@@ -357,8 +358,8 @@ EventEmitter.prototype.removeAllListeners = | |
return this; | ||
}; | ||
|
||
EventEmitter.prototype.listeners = function listeners(type) { | ||
const events = this._events; | ||
function _listeners(target, type, unwrap) { | ||
const events = target._events; | ||
|
||
if (events === undefined) | ||
return []; | ||
|
@@ -368,9 +369,18 @@ EventEmitter.prototype.listeners = function listeners(type) { | |
return []; | ||
|
||
if (typeof evlistener === 'function') | ||
return [evlistener.listener || evlistener]; | ||
return unwrap ? [evlistener.listener || evlistener] : [evlistener]; | ||
|
||
return unwrap ? | ||
unwrapListeners(evlistener) : arrayClone(evlistener, evlistener.length); | ||
} | ||
|
||
EventEmitter.prototype.listeners = function listeners(type) { | ||
return _listeners(this, type, true); | ||
}; | ||
|
||
return unwrapListeners(evlistener); | ||
EventEmitter.prototype.rawListeners = function rawListeners(type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to not make this public? I don't like to expose internal details of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the question is whether it's beneficial to hide these internals or not? I've already seen lots of user-code reaching into Having this be behind a method at least gives us some control over the internals that users accessing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW there's good discussion on this in #6881 where the change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you link some examples which is not
Agreed but I would prefer to have it private. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've seen it in private codebases for this particular use case, described by @ChALkeR in the PR linked above:
As an example out in the wild, this module broke after the change to https://github.com/timoxley/overshadow-listeners/blob/master/index.js Also, Ultron is subtly broken if one adds the same handler as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think this should be exposed for API completeness (for the reason in the @ChALkeR quote above); and that there is code in Node core that uses this should be a good indicator that there are valid use cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see it coming, but changes to how once events are handled can potentially translate to breaking changes if this api is exposed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lpinca can you please articulate this more? What would change on how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will probably never happen but imagine that we will switch to an object to handle events, for example: { once: false, listener }
Yes, the difference is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However we can't currently touch |
||
return _listeners(this, type, false); | ||
}; | ||
|
||
EventEmitter.listenerCount = function(emitter, type) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3354,12 +3354,6 @@ void SetupProcessObject(Environment* env, | |
env->SetMethod(process, "_setupNextTick", SetupNextTick); | ||
env->SetMethod(process, "_setupPromises", SetupPromises); | ||
env->SetMethod(process, "_setupDomainUse", SetupDomainUse); | ||
|
||
// pre-set _events object for faster emit checks | ||
Local<Object> events_obj = Object::New(env->isolate()); | ||
CHECK(events_obj->SetPrototype(env->context(), | ||
Null(env->isolate())).FromJust()); | ||
process->Set(env->events_string(), events_obj); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this cause a perf regression? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I can tell, since we create it in the |
||
} | ||
|
||
|
||
|
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.
Hmm, shouldn't this be
{string|symbol}
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.
It matches the rest of the docs and it is at least partially correct. We don't validate the input so if they pass in a number, it'll get automatically converted to a string.
Either way, if we're changing this then it should be changed in all of the docs.
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.
I've filed #17657 to track that. This again LGTM.