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

[api] Add removeListenersByContext(context) method #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

onlywei
Copy link

@onlywei onlywei commented Jul 26, 2018

I find it very convenient when cleaning up my EventEmitters to remove all listeners that were added with a specific context. This allows me to avoid having to save bound references to my callback functions for use later in my destroy lifecycle methods.

@onlywei
Copy link
Author

onlywei commented Jul 26, 2018

Hmm, not really sure what this username and accessKey stuff for SAUCE is all about. Unit tests pass for sure.

@lpinca
Copy link
Member

lpinca commented Jul 28, 2018

Encrypted variables are not available to untrusted builds such as pull requests coming from another repository so don't worry about that.

Can we find a better name? Like, I don't know, removeListenersByContext(ctx) or simply removeListeners(ctx), other suggestions are welcome but I think removeContext(ctx) is not a good name.

@onlywei onlywei changed the title [api] Add removeContext(context) method [api] Add removeListenersByContext(context) method Jul 29, 2018
@onlywei
Copy link
Author

onlywei commented Jul 29, 2018

@lpinca Thanks. Method renamed.

@lpinca
Copy link
Member

lpinca commented Jul 29, 2018

@onlywei can't this be done with something like this?

for (const event of ee.eventNames()) {
  for (const listener of ee.listeners(event)) {
    ee.removeListener(event, listener, context);
  }
}

I understand it is inefficient but I wonder if the suggested API will ever be used in hot paths. I'm a bit reluctant about adding this method because:

  • The feature it adds can be easily obtained with the already existing methods.
  • It is another divergence from the Node.js EventEmitter.
  • It increases the size of the library.

@onlywei
Copy link
Author

onlywei commented Jul 30, 2018

When using EventEmitter with any sort of browser-side framework such as Angular or React, a common pattern is such:

export default class MyReactComponent {
  componentDidMount() {
    // someEE is passed in as a prop here
    this.props.someEE.on('someEvent', () => doSomething(), this);
    // ... attach more listeners ...
  }

  componentWillUnmount() {
    this.props.someEE.removeListenersByContext(this);
  }
}

Angular components have a similar lifecycle using with $onInit and $onDestroy methods.

If we are not destroying our components very often, then I guess this would not be a hot path.

However, imagine that you have a list of components that is paginated and maybe can display hundreds of items per page. In this scenario, every time you paginate or sort or filter, these hundreds of items will destroy themselves and then get re-created.

Is that "hot" enough for this library? I believe only you can decide :)
Please let me know your decision. I will adjust my coding in each of my components based on your decision.

@lpinca
Copy link
Member

lpinca commented Jul 30, 2018

It would be nice to see a benchmark.
cc: @3rd-Eden

@3rd-Eden
Copy link
Member

I'm indifferent about this addition. I think that most people are now using fat arrows to have the correct context by default so I don't consider this feature that heavily used anymore.

@onlywei
Copy link
Author

onlywei commented Jul 31, 2018

@3rd-Eden Using fat arrow functions only solves the addEventListener part, but it does not solve the removeListener part of a component's lifecycle since the lifecycle functions are different methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants