Repository navigation
events: remove reaches into _events internals #17440
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra method to EventEmitter: wrappedListeners.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -574,6 +574,15 @@ 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`). | ||
|
Member
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.
Contributor
Author
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.
Member
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.
Contributor
Author
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
Member
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. |
||
|
|
||
| [`--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; | ||
|
Member
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. Why is this necessary? It seems to me that it used to be necessary for
Member
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 we use a private
Contributor
Author
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 issue is that this has been public for a while anyway. I'm just adding it onto the prototype since there are packages out there that use EventEmitter without using The reason it wasn't there before — at least as far as I understand — is that there were issues with performance around it.
Member
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. Oh ok then. |
||
| 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) { | ||
|
Member
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
Contributor
Author
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
Contributor
Author
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
Member
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.
Contributor
Author
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 https://github.com/unshiftio/ultron/blob/336c789616db9ca3f164f4e5f15ed78f49d528da/index.js#L105-L111
Member
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.
Member
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.
Member
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
Member
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
Member
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 |
|---|---|---|
|
|
@@ -56,15 +56,15 @@ const realRunInThisContext = Script.prototype.runInThisContext; | |
| const realRunInContext = Script.prototype.runInContext; | ||
|
|
||
| Script.prototype.runInThisContext = function(options) { | ||
| if (options && options.breakOnSigint && process._events.SIGINT) { | ||
| if (options && options.breakOnSigint && process.listenerCount('SIGINT')) { | ||
|
Member
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 make |
||
| return sigintHandlersWrap(realRunInThisContext, this, [options]); | ||
| } else { | ||
| return realRunInThisContext.call(this, options); | ||
| } | ||
| }; | ||
|
|
||
| Script.prototype.runInContext = function(contextifiedSandbox, options) { | ||
| if (options && options.breakOnSigint && process._events.SIGINT) { | ||
| if (options && options.breakOnSigint && process.listenerCount('SIGINT')) { | ||
|
Member
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. same here. |
||
| return sigintHandlersWrap(realRunInContext, this, | ||
| [contextifiedSandbox, options]); | ||
| } else { | ||
|
|
@@ -95,14 +95,7 @@ function createScript(code, options) { | |
| // Remove all SIGINT listeners and re-attach them after the wrapped function | ||
| // has executed, so that caught SIGINT are handled by the listeners again. | ||
| function sigintHandlersWrap(fn, thisArg, argsArray) { | ||
| // Using the internal list here to make sure `.once()` wrappers are used, | ||
| // not the original ones. | ||
| let sigintListeners = process._events.SIGINT; | ||
|
|
||
| if (Array.isArray(sigintListeners)) | ||
| sigintListeners = sigintListeners.slice(); | ||
| else | ||
| sigintListeners = [sigintListeners]; | ||
| const sigintListeners = process.rawListeners('SIGINT'); | ||
|
|
||
| process.removeAllListeners('SIGINT'); | ||
|
|
||
|
|
||
| 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); | ||
|
Member
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?
Contributor
Author
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 |
||
| } | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,3 +82,14 @@ function listener2() {} | |
| const s = new TestStream(); | ||
| assert.deepStrictEqual(s.listeners('foo'), []); | ||
| } | ||
|
|
||
| { | ||
| const ee = new events.EventEmitter(); | ||
| ee.on('foo', listener); | ||
| ee.once('foo', listener); | ||
| const wrappedListeners = ee.rawListeners('foo'); | ||
| assert.strictEqual(wrappedListeners.length, 2); | ||
| assert.strictEqual(wrappedListeners[0], listener); | ||
| assert.notStrictEqual(wrappedListeners[1], listener); | ||
| assert.strictEqual(wrappedListeners[1].listener, listener); | ||
| } | ||
|
Member
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 some more tests?
|
||
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.