Event Emitter

[heapwolf]

Hello All. Here is the eventEmitter that I've been working on. I have added a few simple features, none of which are breaking changes. These changes (namespace/wildcards etc) have been documented in doc/api/events.js. This code is passing all release tests.

@ry, as discussed there does not seem to be a noticeable increase in performance of the ab tests despite the performance increase of the javascript in isolation.

[ry]

do you have tests?

benchmarks appear to be on par with HEAD

nits: there's a few EOL whitespace errors. you should wrap the lines in the docs.

[heapwolf]

I have fairly comprehensive tests, but they are written using nodeunit. i'm not sure how you guys feel about that. They can be found here (https://github.com/hij1nx/EventEmitter2/tree/master/test). I can refactor the dependancy out of these tests if you want to integrate them into the project.

[mikeal]

v8 inlines the constructor for new MyObject() which would explain the perf difference.

[mikeal]

+1 to this patch including tests.

I'm +0 on the names of things ;) My immediate reaction to the *Any names and .un() is negative but to be honest I can't think of a better name so I don't have any room to complain.

[ry]

@hij1nx we'll need tests using the assert module - but let's see if we actually want this before making you do a bunch of work.

@pquerna, @koichik, @bnoordhuis, @felixge, @isaacs what are your opinions?

[mikeal]

I'm +0 on namespaces.

+1 on the catchall listener, there was a thread about that a while ago that was pretty unanimous in favor of adding it.

[isaacs]

I'll review the code changes more closely in the next day or two. Feature-wise, it all looks good to me. I'm glad we're going about this carefully, this path is hot like magma.

[indexzero]

+1 Clearly; obviously I'm a little baised since I helped write it :-D

@isaacs This pull-request is the result of a lot of concerted effort from @hij1nx @DrJackal and myself, the magma will flow in the hot paths (i.e. w/o { wildcard: true }) as it did before

@mikeal Namespaces are really useful, have you taken a look at how they are used in [email protected]?

[mikeal]

I know namespaces can be useful, it's not a -1, it's a +0. It's something I won't use but don't dislike and generally don't have a strong opinion about one way or the other :)

[bnoordhuis]

-1

Namespaces: on the fence, convince me.

onMany: somewhat useful, I suppose, but something you can stitch together yourself with little effort.

Wildcards: this has been hashed out on the mailing list but the tl;dr version is that no one could present a good use case except debugging and that's something you can duct-tape together in 2 minutes.

function log_events(ee) {
  var fun = ee.emit;
  ee.emit = function() {
    console.error(arguments);
    return fun.apply(ee, Array.prototype.slice.call(arguments));
  };
  return ee;
}

All in all it seems like a lot of extra code for little gain.

[indutny]

+1
I thought about namespaces since my acquaintance with nodejs. And wildcards... Oh, I can't even describe how cool it'll be to see that in core.

[indexzero]

@bnoordhuis Have you taken a look at how [email protected] works? It takes serious advantage of the wildcard events. There is a recent post on the Nodejitsu blog which I suggest reading to get a better understanding of the benefits here:

http://blog.nodejitsu.com/distribute-nodejs-apps-with-hookio

[mikeal]

the question is less about how useful it would be to third party modules and more about what we would use it for in core. if there isn't a single usecase for it in core then it probably doesn't belong.

cool stuff doesn't have to be in core, in fact there is an argument to be made that more cool stuff in third party modules is a better thing for the community long term.

that said, a listener for all events has been brought up many times as a need for debugging.

[bnoordhuis]

@indexzero: Okay, I see how wildcards and namespaces can be useful. But I agree with @mikeal that it's something that core doesn't have any use for (right now anyway) so it should stay in user land. Let's say -0.5 instead of -1.

[jamesonjlee]

You can do the namespacing with current EE, foo::bar is just a simple string, the wildcard feature is what is new. foo::*
or even better. *::error

[heapwolf]

@bnoordhuis @mikeal -- I am a proponent of a sparse core <3

But I also believe namespaces are a succinct and well known mechanism for event characterization. As we build more complex event based systems using node, we should establish simple and well known conventions for describing them. This patch provides basic progressive enhancement that does not impose upon the existing node programming style or APIs. Therefore I'm compelled to propose it as a core commit.

[coverslide]

As long as there's no breakage, and no major performance penalty, I don't see why it shouldn't be in core. It is incredibly useful, and the simple matter that core modules don't use it doesn't mean they can't benefit from them in the future.

[mikeal]

@hij1nx "As we build more complex event based systems using node".

That's the issue though isn't it. This is intended for use by people using node, not by core itself.

It's not that I don't think this pattern is a good one, but adding it would be blessing it. We're trying not to do that.

Blessing a particular pattern can reduce innovation on top of node.js in the rest of the community, and it's still very early.

If this patch made it in hardly anyone would use your eventemitter2 library because it's primary offerings will be in core. That library is where most of the current innovation is happening around the patterns you propose here, if nobody is using it where does that innovation happen?

And if in 6 months you have a great idea that compliments or potentially changes some of this stuff you won't have a lot of users to give you feedback and you'll just be trying to put more of it in to core and it'll be even harder and hit considerably more skepticism.

[tj]

+0 from me. Doesn't really matter to me either but there are plenty of "nice to have"s that could be in core and gain wider adoption, a better sprintf implementation, blah blah.

[mranney]

I really like EventEmitter, and I think it's a useful abstraction and guide for how to write node programs. I like these improvements, but I'm torn about whether I think they should go in core. I would probably not use this if it weren't in core, but I probably would if it were. I think the real question is whether we think this is how people should be writing node programs. By putting it in core, it basically says, "you should do it this way."

For listeners that can catch multiple events, it'd be useful if they were passed the original event name that matched.

[heapwolf]

The EventEmitter2 library came from a need, the need to be more expressive when interacting with events. Since node APIs make heavy use of EventEmitter. I believe this is less endorsing a pattern than maturing an existing one.

@mranney for the case where a wildcard is used, this.event will provide the actual name of the event fired.

[codenothing]

function MyObject(params){
    EventEmitter.call(this);
    this.params = params;
    this.init();
}

util.inherits(MyObject, EventEmitter);
MyObject.prototype.init = function(){
    // Do something to initialize your object
};

I personally don't like the the above example, but I see a lot of third-party modules blindly eventing their objects. I think there are too many namespaces included in this commit, and there might be a number of unknown conflicts with older modules.

[heapwolf]

The EventEmitter2 library came from a need, the need to be more expressive when interacting with events. Since node APIs make heavy use of EventEmitter, I believe this is less endorsing a pattern than maturing an existing one.

@mranney for the case where a wildcard is used, this.event will provide the actual name of the event fired.

Edit: @mikeal i've been intentionally quite conservative with this work, and i will continue to be, i think it should stay simple.

@codenothing you lost me with " I think there are too many namespaces included in this commit, and there might be a number of unknown conflicts with older modules." =)

[isaacs]

@hij1nx I think the issue is that "init" is a fairly common method name, and calling "this.init()" might not call EventEmitter.prototype.init. How badly does it affect the benchmarks to change that to EventEmitter.prototype.init.call(this);, or just inlining the "init" functionality into the EventEmitter constructor?

[isaacs]

(that is, "the issue" = "the thing that @codenothing is complaining about". Please correct me if I missed the point of your comment, @codenothing.)

[heapwolf]

his example assumes you need to call it, you actually should never need to call init(), init gets called for you...

this.events || this.init();

Edit: it has become quite clear to me over time that people are often unsure how inherits works with relationship to constructors, hence the above code occurs when needed to reconcile the problem. /cc @isaacs @codenothing

[indexzero]

I was reluctant to bring this up because it is clearly part of a larger discussion. My apologies to all for such hubris.

From what I've gleaned from @ry, a real goal for core is to be able to facilitate somekind of distributed communication between node processes. Clearly the .fork() API is a step in this direction. Working with peer processes and overall how a supervision tree (or perhaps undirected / directed graph) of processes would communicate is unclear. It is true that namespaces are not always necessary, for things such as data, request, connection events, but having been working intimately with a distributed network of node processes recently I see true value (within core) in the semantics that wildcard, and namespaced events introduced.

Specific replies

@mikeal We have a blessed version of IPC in .fork(), I'm not questioning that implementation or the decision to have it in core, but I think that this EventEmitter would provide patterns necessary to really leverage the existing core IPC APIs. I'm never one to want to stifle innovation, but the features here are not revolutionary: it performs a trie-like exploration of an n-ary tree with arbitrary depth (i.e. Object literal) keyed on components of an event name. We had tried alternative implementations that were proven to be less performant, if you're curious I have a gist of that discussion: https://gist.github.com/1036944. (Also see my comment to @mranney)

@mranney Why would you not use this if it wasn't in core? The separate EventEmitter2 module will continued to be maintained by @hij1nx and the rest of Nodejitsu because it adds some sugar for using it in the browser that doesn't belong in node for those who don't want to use something like browserify.

@hij1nx I believe that @isaacs @codenothing are correct. To my understanding when the MyObject constructor function is invoked, the init() method has already been overridden on the "child" prototype, so the call to this.init and (less likely) the call to this.setConf will invoke the methods on the "child", not the methods on EventEmitter.prototype as intended. Given that init and setConf are somewhat common names, we could invoke the prototype or indicate the reserved status through _init and _setConf as is with the HTTP _setHeader and _renderHeaders API
https://github.com/hij1nx/node/commit/edfe18f17cea4239fa72ebc46383dbb779ef54e8#L0R23

[codenothing]

Sorry, should have been more clear. What I'm getting at is there is potential for many naming conflicts.

• init
• setConf
• defaultMaxListeners
• wildcard
• delimiter
• listenerTree
• many
• event
• un
• unAny

Are all new properties created with this commit. For such a highly extended object, there are bound to be naming conflicts within existing modules. I like the addition, it just needs to be well documented.

[tj]

@hij1nx haha yeah definitely a bit different. It's a bit of a tough call, saying not to include something unless node core uses it doesn't really work either because there are still quite a few things core it-self doesn't really provide. I guess the major thing is that if it can be done reasonably as a third-party module or not. It's cool it just seems like a similar to something like the request module where (in most cases at least) is just extending and the API additions are still optional but where do you draw that line. I dont know a good answer for that either, just seems like var EventEmitter = require('ee2') or something isn't much of a problem

[heapwolf]

@visionmedia you raise some great points. var EventEmitter = require('ee2') is not much of a problem, that's currently how its used.

As a community, I believe we are motivated, not just to keep core sparse, but to keep the mental model simple and let growth emerge at the perimeter. So I believe the question in regards to this patch is centered around how disruptive these changes are to the mental model of node developers and how it affects innovation potential. It feels like either an executive decision at this point, or at least a core committers decision.

[aredridel]

Wow. Good stuff. My gut reaction to wildcards is "I'm not going to enjoy a lot of what people do with them", so +0 from me on that, but namespaces feel relatively right.

[rauchg]

-1 on changing one of the core-est parts of node for something that hasn't really seen overwhelming adoption or interest.
+1 on waiting longer and reviewing this again.

[mraleph]

@mikeal constructor inlining is not implemented yet. only normal calls are inlined by crankshaft.

[Gozala]

-1 on making this part of core until there is an actual need for these features in the core itself.

Making this part of the core puts all other alternative patterns into a very weak position, which I don't think is beneficial for the community, at least not yet.

[heapwolf]

@Gozala the idea that something is only useful if its useful internally, is itself debatable as we've already discussed. Adding value to user-land also has validity.

[Gozala]

@hij1nx

@Gozala the idea that something is only useful if its useful internally, is itself debatable as we've already discussed.

That's why I'm expressing my opinion on that.

I just think in general it's better to defer blessing any particular pattern until that becomes a blocker (core needs it).

[heapwolf]

@Gozala, of course its fair to state your case! but again, i do not believe this patch is a blessing, i believe it lends maturity and completeness to the existing pattern. Having named events and being able to classify them are not such disparate ideas.

[indexzero]

I'd like to elaborate on the scenario that I outlined in my previous comments in the interest of both those now joining the comment thread, and solidifying a high-level thought into a concrete example. That is, what does the current .fork() IPC mechanism look like once it becomes more widely used and what are scenarios that we want to support?

If you have not seen @isaacs talk on "Node digs DIRT", now would be a good time.

I've prepared a sample centered around DIRT / ETL based applications for which node has clear limitations from the V8 heap. If we want to support these scenarios, this should go into core. In my opinion, this isn't some abstract user-land problem, this is grunt-work data processing that we need to be capable of doing:

https://gist.github.com/1122318

[aheckmann]

-1 The only feature here that I recall wanting is namespaced events and that only one time for a very specific use case.

+1 For leaving it in userland.

[heapwolf]

damn. @indexzero presents a very compelling argument. a lot of -1's feel nullified.

[felixge]

-1 on merging this as a whole.

This is too big of a change to be reasonably discussed by the amount of people involved in this discussion. We need to split this effort into multiple patches which we can discuss separately, one after another.

I therefor suggest that we focus on the catch-all listener feature first. I feel like:

• Namespaces and wildchars can be implemented on top of it in userland
• We probably have more core people who would agree to it than for the other feature proposals
• A core use case would be to forward unknown events in .pipe() (let's not discuss that for now, it's merely a potential example)

--fg

[heapwolf]

I agree with @felixge. this is a big change. It needs to be broken into smaller parts. There is a lot of knee-jerk reaction and a lot of noise in this thread. This needs to go to a smaller group.

[isaacs]

Ok, had a chance to read through the changes more carefully. I think that this is overall a very good patch, and the benchmarks seem to be a bit faster than on master, which is always nice. The code is very understandable, and seems to work as documented.

Some relatively minor issues:

1. Prefer named functions. That is function growListenerTree(type, listener) { rather than var growListenerTree = function(type, listener) {. Makes stack traces nicer.
2. Some trailing spaces that should be cleaned up. Certainly not the only make jslint offense in node's codebase, but it'd be good to reduce those rather than increase :)
3. EventEmitter.prototype.init should not be exposed (discussed above).
4. Seems like onAny should return this, to match the pattern of once, on, etc.
5. I agree that it would be nice to split into a few commits if it can be done in a nice atomic way, but I don't think that's such a big deal. It's not that big, really.
6. I'd really like it if we could just pick a delimiter and wildcard, rather than making that configurable. Everyone I've seen using EventEmitter2 seems to prefer ":" as the delimiter and "*" as the wildcard, and it seems like the natural fit.

[ry]

I'm +1 ... I think.

[slaskis]

I've personally always liked the namespaces in jQuerys events but I guess it might be a bit limited as it assumes a certain style?

But an example could be:

// Add two events namespaced to 'http' 
server.on("connection.http",onConnect).on("close.http",onClose) 
// Removed all events namespaced with 'http'
server.removeEventListener(".http") 

I guess it's the ":" (or even worse "::") that looks like dirty xml namespaces to me when the "." looks more like clean js/json to me...but I thought I'd just throw this into the mix

[chjj]

Namespaces aside, if this does go in eventually, I have one small suggestion. Rename the removeListener alias, un to off. It corresponds to on rather nicely, and if you know what on does, you could figure out what off does. un seems kind of unintuitive.

[tj]

@chjj++

[mikeal]

this bikeshedding is now out of control.

i think @felixge is right, we need to break this up in to a few different patches, it would make this a lot easier.

[heapwolf]

@mikeal, i agree. but I also dont agree. @isaacs, i get on those changes immediately. @ry let's put the patch in =)

@chjj & @visionmedia this is actually a very common short cut in browser frameworks, thats why i adopted it, but i also really like off.

[heapwolf]

@slaskis . is the default delimiter, which is configurable. @isaacs, I agree, perhaps you or @ry could just make that decision.

[tj]

really? I've never seen .un(). I agree with the guys though, the patch is a little unfocused and could easily be broken up. The amount of talk in here really shows how badly we need some kind of description for what gets in core though haha, whatever that is.

[slaskis]

@hij1nx haha, yeah, after posting (shame on me) I saw that in the diffs too.

Another difference is the implicit wildcard i.e. ".http" instead of "*.http". Might be slightly harder to read perhaps, but it sure looks clean if you ask me...

[heapwolf]

@visionmedia with all due respect, i whole heartedly disagree that it is unfocused, in fact i believe it to be exactly the opposite. It is laser focused on the expressiveness of events, specifically how someone will be able to characterize them.

[tj]

@hij1nx as a whole, but each method etc could be a commit that people could individually be accepted/commented on etc

[heapwolf]

@visionmedia well, the commit is actually pretty small, we're talking about + 394 additions, less after my newest optimizations/simplifications (shows another performance improvement at master).

[mikeal]

size of this discussion is dwarfing the size of the commit. breaking it up reduces the surface area for discussion :)