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: adding multiple listeners at once #15697

Closed
silverwind opened this issue Sep 30, 2017 · 15 comments
Closed

events: adding multiple listeners at once #15697

silverwind opened this issue Sep 30, 2017 · 15 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter. feature request Issues that request new features to be added to Node.js.

Comments

@silverwind
Copy link
Contributor

I often find myself in need of registering to multiple events using the same handler. .on currently only supports on event per call, so this most concise way to write it is

const errorHandler = (err) => {
  // handle error
}

server.on('error', errorHandler)
server.on('clientError', errorHandler)
server.on('tlsClientError', errorHandler)

Ideally, I think .on and possibly .once should provide a way to add multiple listeners in one call, like for example using jQuery-style space-separated event names:

server.on('error clientError tlsClientError', (err) => {
  // handle error
})

Above would be a breaking change for event names containing spaces, so an alternative could be to take the event names from an array:

server.on(['error', 'clientError', 'tlsClientError'], (err) => {
  // handle error
})
@silverwind silverwind added events Issues and PRs related to the events subsystem / EventEmitter. feature request Issues that request new features to be added to Node.js. labels Sep 30, 2017
@addaleax
Copy link
Member

Just thinking about the polymorphism that comes with allowing arrays – would emitter.onAnyOf(array, fn) (or any other new method name) work for you just as well?

@jasnell
Copy link
Member

jasnell commented Oct 1, 2017

I've often thought about making improvements to this area but there are some challenges to keep in mind. I ran into some of these when introducing the prependOn and prependOnce methods and primarily deal with backwards compatibility and compatibility with things like the standalone readable-streams module. If changes are made, I strongly suggest adding new methods and not touching the existing methods.

Getting back to the original question, the approach I've considered is more along the lines of....

emitter.addListeners({
  error() { /* ... */ },
  clientError() { /* ... */ },
   tlsClientError() { /* ... */ }
});

emitter.addOnceListeners({
  error() { /* ... */ },
  clientError() { /* ... */ },
   tlsClientError() { /* ... */ }
});

function fn() {}

emitter.addListeners({
  error: fn,
  clientError: fn,
  tlsClientError: fn
});

@silverwind
Copy link
Contributor Author

silverwind commented Oct 1, 2017

Just thinking about the polymorphism that comes with allowing arrays – would emitter.onAnyOf(array, fn) (or any other new method name) work for you just as well?

I'm not really happy with introducing additional methods. What issues are foreseeing when adding array support?

Currently, when registering a listener with an array, a implicit toString conversion is ran, so adding a ['a','b'] listener results in a event named 'a,b' being actually registered, which is likely not what the user wanted. Still I'd say a change to array handling should be semver-major.

@mscdex
Copy link
Contributor

mscdex commented Oct 1, 2017

@silverwind There could be performance issues due to the differing types being passed to the same function in addition to the added argument type checking.

If this should get implemented, I would also prefer to see it as a separate function.

@Giovarco
Copy link

Giovarco commented Oct 2, 2017

To be honest, I think that this is a common issue to everyone. If we can agree on the relevance of the issue, maybe I could consider trying to work on this.

By the way, I took at look at the code. It does not seem impossible to change, but this is an opinion of a beginner. Do you think that this is a beginner-level issue?

@jasnell
Copy link
Member

jasnell commented Oct 2, 2017

The key issues I see with allowing the polymorphic signature are Performance and Discoverability... that is...

  1. Performance (as @mscdex points out). The additional type checking does not come free.
  2. Discoverability... that is, one would have to know exactly which versions of Node.js the polymorphic signature has been ported to in order to reliably use it (vs. just checking for the existence of the new method)

There is another approach to this that I've been considering... the ability to register a generic event sink for any event emitter... e.g.

function mySink(event, ...args) {
  switch (event) {
    case 'foo': console.log('foo'); break;
    case 'bar': console.log('bar'); break;
  }
}

const ee = new EventEmitter();
ee.addEventSink(mySink);
ee.emit('foo');
ee.emit('bar');

@silverwind
Copy link
Contributor Author

I think I'm convinced that the downsides of polymorphism are outweighting the benefits. So for the question on what to implement, I'd propose:

  • emitter.onMultiple(events, handler), where events is an array and handler recieves ...args
  • emitter.onAny(handler) where handler receives event, ..args (@jasnell's sink idea)

I'm not sure of the value of similar methods for .once, so I think it's better left out for now.

@Giovarco I guess this shouldn't be too hard to implement.

@jasnell
Copy link
Member

jasnell commented Oct 2, 2017

If we went with the handler approach, there'd definitely be no need for a once counterpart.

@Giovarco
Copy link

Giovarco commented Oct 2, 2017

@silverwind Ok then tomorrow I'll give it a shot. emitter.onMultiple(events, handler) use is ok, but I didn't quite understand the use of emitter.onAny(handler).

@simonkcleung
Copy link

If you don't concern the performance, you may use ES6 Proxy to achieve what you want in this way:

const EE = require("events");

function EP(target){
	return new Proxy(target, proxyHandler);
}
const proxyHandler = {
	set(target, name, handler){
	target.addListener(name, handler);
	}
};

let e = new EE();

let p = EP(e);

p.error1 = p.error2 = p.error3 = function(){
	console.log("This is an error");	
}

e.emit("error1");
e.emit("error2");
e.emit("error3");

@Giovarco
Copy link

Giovarco commented Oct 3, 2017

To be honest, the event modules is a little bit murky for me. I did not find a "node API for developers" that explain the inner workings of the module. Anyway, this is how I tried to add the feature:

events.js

EventEmitter.prototype.onMultiple = function onMultiple(eventList, handler) {

  for(let i = 0; i < eventList.length; i++) {
    this.on(eventList[i], handler);
  }

};

This is my test:

test-event-emitter-onMultiple.js

'use strict';
const common = require('../common');

// This test ensures that it is possible to add a listener to multiple events at once

const assert = require('assert');
const EventEmitter = require('../../lib/events');
const myEE = new EventEmitter();

async function main() {

    // Input
    const inputArray = ["a", "b", Symbol('symbol')];
    const emptyFunction = function() {};

    // Test
    await myEE.onMultiple(inputArray, emptyFunction);
    const outputArray = await myEE.eventNames();
    
    // Assert
    await assert.deepStrictEqual(inputArray, outputArray, "The listener did not get binded to multiple events");
}

main()

I suspect that I'm far from a good implementation, but it works at least. Please don't be too strict with me ^_^"

@bnoordhuis
Copy link
Member

Anyone have examples other than the one @silverwind posted? Going over my own code, the places where I add the same listener for different events can be counted on one hand.

@silverwind
Copy link
Contributor Author

Anyone have examples

For core modules, it's probably only useful for error handling. One more example:

process.on('uncaughtException', handler)
process.on('unhandledRejection', handler)

Other than that, I could've used in on modules that fire many different events like chokidar.

@Giovarco
Copy link

@bnoordhuis I you're working on implementing this feature, please read this #16169 . I'm already working on it, so you can propose changes and we don't get duplicate code.

@silverwind
Copy link
Contributor Author

Closing this for lack of support. Also see #16169 (comment).

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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

7 participants