I have twitted already that it sucks as it is, and while it's really trivial to implement the same for any browser, I believe many mistakes have been made about the API. Here the list:
- on() is a shortcut for addListener but there is no off() as removeListener's shortcut (inconsistent)
- add/removeListener is not DOM friendly, we cannot reuse an EventEmitter in the browser without double checking if those methods exist
- removeAllListeners() accepts no arguments and cleans up the whole emitter but there is no way to retrieve all listeners via listeners() passing no arguments (again, inconsistent)
- there's no possibility to use handleEvent property, as already defined in the EventListener, once again objects are not reusable between client and server
- no duplicated checks for both addListener and removeListener, this is totally inconsistent against DOM EventTarget plus I found it kinda hilarious since there is also a max number of listeners allowed defined via setMaxListeners method ... if we add same listener twice, it's fired twice ... a non-sense for those coming from the web. We also need to removeListener twice or more 'cause we have no idea if same listeners has been added by mistake twice so ... it's just screwed up, removing a specific listener may be not enough and there is no easy way to check if the listener has been removed or not
- there is no interface to define events plus the single argument is not a DOM Event like, not even an object with data property and at least a type that points to the event name ... once again, not possible to reuse anything on DOM world
Why All Of This Is Bad
node.js is bringing a lot of non JavaScripters and non browser friendly JS developers into its community and this is the good part. What is absolutely bad is that if node.js won't be minimally aligned with the rest of the code in the browsers out there our life as "one language everywhere" will become harder than ever.I have personally created wru which runs in all browsers and many server side JS environments but what I would like to avoid is to write twice any sort of test because of weirdly implemented APIs.
If it's about shortcutting, as example, on() and off() are more than welcome but why on earth the long version should be addListener rather than addEventListener?
Why events passed as objects since ever in client JS should be passed as string with optional extra data as second argument?
Where are defaults control over events fired across more listeners?
Why on earth handleEvent is not supported and a listener can be only a function which most likely will require a bind to something else?
Why is it possible to erase all listeners without even knowing "who set them where" but it's not possible to retrieve all of them so that at least we could act accordingly with the event name following some rule rather than "just remove them all" ?
I hope somebody will agree with me, considering flakey points I have already described and changing or improving ASAP this EventEmitter API ... it would be sooooooo coooool to be aligned in both worlds for at least the most used pattern/procedure ever in JS world, don't you say? Thanks for your attention.
Take a look at EventEmitter2 (https://github.com/hij1nx/EventEmitter2). This project is abstracting a more feature-laden EventEmitter. Also, .once will allow an EventEmitter to be set and once it is executed, that particular event will be unregistered.
ReplyDeletethanks Chris, I'll have a look.
ReplyDeleteonce() is there already and it's the only thing, together with on() that I liked ... everything else should be more aligned with what we have since ever in client JS world ... I just don't like the non-DRY approach current EventEmitter took
Can't say I agree the duplicate check is needed. What kind of code do you write, that adds multiple event handlers by mistake? The code should be structured in a way that makes rreverting side effects just as easy, as commiting them.
ReplyDeletea module that exports an EventEmitter could be used by other two modules that depends on another shared module too ... in this case
ReplyDeleterequire("stuff").event.addListener("connect", require("my net").debugConnection);
could be already a case where you may have side effects if another module included in the script does the same.
I program in a way that if I remove a listener I wanna be sure 100% that has been removed, not because I am the owner of the EventEmitter, because that's how it should be.
Nobody does this to remove a listener in node.js, isn't it?
em.listeners("type").filter(function (listener){
return listener === nmsp.callback;
}).forEach(function (listener) {
em.removeListener("type", listener);
});
... I mean, what the hell is that? why is even possible to add sam listener to the same event type?
It's not a case that in DOM world this has never been possible plus has never been needed, you know what I mean?
On github direct, I comented (few months) ago about my surprise with node.js. Code inside is definitely *not* up to the standards of let's say jQuery.
ReplyDeleteThat was not received very well, to say at least. I tried to point out what's wrong, but those guys seem a bit touchy on the subject of the lack of quality javascript code inside node.js
Which is unfortunate. Concept is good and product is good. But node.js, indeed looks like *not* top quality js code.
I agree, the source code of EventEmitter is really not top quality and other JS native modules too.
ReplyDeleteI am thinking to rewrite the EventEmitter and leave the API as it is but at least make it better, quality speaking, smaller and hopefully faster.
Still, this post was not about EventEmitter itself, rather about its API which is inconsistent in few places and not aligned with good old JS behaviors
I agree that it is inconsistent with the browser, but that was never the intent in Node. EventEmitters vs EventTarget is a big gap in philosophy. One of them packs everything on a single object, one uses multiple arguments. One is used in situations where events may not be known before hand (a lot of the EventEmitter2 wildcard stuff comes to mind). Changing an EventEmitter to an EventTarget would be catastrophic for Node's idea of a single callback with error first argument, and you would go back to the nightmare of onsuccess/onfailure and manually checking if it met failure conditions in both. While both are used for event delegation, EventTarget also has the concept of bubbling/capturing/preventingDefault/etc., which event emitters do not. I would not try to reconcile this large gaps by forcing one of the two to act as the other.
ReplyDelete