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

events: allow unwraping #once handler in listeners #7172

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 6, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

events

Description of change

De-semver-majorified variant of #6881: Instead of changing the return values of .listeners() unconditionally, adds an optional parameter that indicates which behaviour is desidered.

For unwrapOnceListeners = true, the array returned by .listeners() will include the handler functions as passed to .once().

For unwrapOnceListeners = false (the default and the current behaviour), the array returned by listeners() will include the wrapper functions used by .once(), not the original functions that were passed in from userland. Use cases for this include temporarily disabling an event by removing all handlers and adding them back at a later point, where it would be undesirable to deal with the original event handlers.

Ref: #6873
Ref: #6881

CI: https://ci.nodejs.org/job/node-test-commit/3659/

@addaleax addaleax added events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 6, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 6, 2016

I'd personally rather take #6881 as a semver major to fix #6873, rather than take on a new API to accomplish the same thing.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

I agree with @cjihrig. I consider the fact that listeners() currently returns the once wrappers to be a bug that simply needs to be fixed. I'm -1 on introducing a new API or argument like this.

@addaleax
Copy link
Member Author

addaleax commented Jun 6, 2016

Hm yeah, so did I until I realized that the API as it exists now has use cases that wouldn’t be covered by the change in #6881, see e.g. this comment. If you think relying on the _events property is okay in those cases, then yeah, I’ll go ahead and close this.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

I'm not convinced that those are cases we should necessarily care about and there are plenty of users who already go directly to _events for edge-case kinds of things (as much as we should discourage that). If new APIs are necessary to support such cases in the future, then so be it, but for this, I think the correct behavior is pretty clear (at least to me it is :-) ...)

@addaleax
Copy link
Member Author

addaleax commented Jun 6, 2016

Okay, I’m closing this then.

@addaleax addaleax closed this Jun 6, 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants