Skip to content
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

EventEmitter: Inconsistent handling of once() handlers #6873

Closed
Flarna opened this issue May 19, 2016 · 5 comments
Closed

EventEmitter: Inconsistent handling of once() handlers #6873

Flarna opened this issue May 19, 2016 · 5 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter.

Comments

@Flarna
Copy link
Member

Flarna commented May 19, 2016

  • Version: 6.2.0 (with references to others)
  • Platform: Windows 64 bit
  • Subsystem: EventEmitter

It seems EventEmitter has an inconsistent handling of listeners installed via once().

I think #5564 was targeting this topic also but was closed because #6394 was merged.

Not sure if this is a but or intended - or if changing this is seen as breaking change or not.
If it is kept like it is I would vote for updating documentation accordingly.

@addaleax addaleax added the events Issues and PRs related to the events subsystem / EventEmitter. label May 19, 2016
@cjihrig
Copy link
Contributor

cjihrig commented May 19, 2016

It was decided in #6394 that the previous behavior was a bug, and therefor not a breaking change.

What did you have in mind as a documentation update?

@Flarna
Copy link
Member Author

Flarna commented May 19, 2016

#6394 has changed/corrected the behavior of signaling which listener has been removed via 'removeListener' event.
The listeners() function has not been changed in #6394, it still returns the wrapped function.

I think we should either document that listeners() is not returning the registered listener or correct it to actually return the listener.

@cjihrig
Copy link
Contributor

cjihrig commented May 19, 2016

Ah, yea, @addaleax pinged @omsmith about that in #5564 (comment).

@jasnell
Copy link
Member

jasnell commented May 19, 2016

to be honest, if we could do so in a way that didn't kill perf I'd be ok with listeners being modified to not return the wrapper given that the wrapper really is an internal implementation detail.

omsmith added a commit to omsmith/node that referenced this issue May 19, 2016
@Flarna
Copy link
Member Author

Flarna commented May 19, 2016

Is it really proven that adding a simple unwrap in listeners() kills performance?

I would wonder that listeners() is an API which is used in performance relevant code as it creates a copy of an array. Quite a lot work - for actually getting list of functions back which has maybe "wrong" entries inside in all node versions till now.

Most use cases I have seen till now are in the end just interested in the size of the array which is available much cheaper using listenerCount() where whether copy nor unwrap is needed.

Not sure if this is the right place to ask: But as this and #6394 are seen as bugs are there also ported back to Node 4?

addaleax pushed a commit to addaleax/node that referenced this issue May 27, 2016
addaleax added a commit to addaleax/node that referenced this issue Jun 6, 2016
@jasnell jasnell closed this as completed in b7a8a69 Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

No branches or pull requests

4 participants