Various events.EventEmitter issues

[silkentrance]

node@v0.13.0-pre

First, it will test for isObject while it should be testing for isArray.

153   else if (util.isObject(this._events[type]))
154     // If we've already got an array, just append.
155     this._events[type].push(listener);

Second, it will initialize _maxListeners to a default of 'undefined'

52   this._maxListeners = this._maxListeners || undefined;

while EventEmitter.defaultMaxListeners would be the better choice. In effect the following code
could then be simplified

163     if (!util.isUndefined(this._maxListeners)) {
164       m = this._maxListeners;
165     } else {
166       m = EventEmitter.defaultMaxListeners;
167     }

to just m = this._maxListeners || EventEmitter.defaultMaxListeners;

Third,

 74   // If there is no 'error' event listener then throw.
 75   if (type === 'error' && !this._events.error) {
 76     er = arguments[1];
 77     if (this.domain) {
 78       if (!er)
 79         er = new Error('Uncaught, unspecified "error" event.');
 80       er.domainEmitter = this;
 81       er.domain = this.domain;
 82       er.domainThrown = false;
 83       this.domain.emit('error', er);
 84     } else if (er instanceof Error) {
 85       throw er; // Unhandled 'error' event
 86     } else {
 87       throw Error('Uncaught, unspecified "error" event.');
 88     }

could be simplified to

76  er = arguments[1] || new Error('Uncaught, unspecified "error" event.');
78 // delete line
79 // delete line
84 // delete line
85 // delete line
87     throw er;

Fourth,

 94   if (util.isUndefined(handler))
 95     return false;

wouldn't

 94  if (!handler) {
 95     return false;
 96  }

be more efficient?

Fifth, again testing for isObject while it should be testing for isArray

120   } else if (util.isObject(handler)) {
121     len = arguments.length;
122     args = new Array(len - 1);
123     for (i = 1; i < len; i++)
124       args[i - 1] = arguments[i];
125 
126     listeners = handler.slice();
127     len = listeners.length;
128     for (i = 0; i < len; i++)
129       listeners[i].apply(this, args);
130   }

And, finally, in

224   if (list === listener ||
225       (util.isFunction(list.listener) && list.listener === listener)) {

it should be documented that list.listener refers to a once only listener and that list could be any one of a function/array/wrapper for a one time listener.

Or not so finally 😁

297 EventEmitter.prototype.listeners = function listeners(type) {
298   var ret;
299   if (!this._events || !this._events[type])
300     ret = [];
301   else if (util.isFunction(this._events[type]))
302     ret = [this._events[type]];
303   else
304     ret = this._events[type].slice();
305   return ret;
306 };

could be simplified by

298  var ret = [];
298.5 var listeners = this._events[type];
299  // delete line
300  // delete line
301 if (util.isFunction(listeners))
302   ret.push(listeners);
304   ret.push.apply(ret, listeners);

[simonkcleung]

First, true. It should use Array.isArray or util.isArray instead of util.isObject.
2nd, you should also beware that this._maxListeners could be zero.

[silkentrance]

Is this a proper configuration for the event emitter, zero maxListeners? Would this not prevent any listeners from being added to the event emitter?

[simonkcleung]

As mentioned in the api document, "Set to zero for unlimited"

[silkentrance]

Never mind 🍄

[silkentrance]

So could EventEmitter please use Array.isArray instead of util.isObject, then?

[jasnell]

The events implementation has been reworked significantly within the io.js stream. Many of these issues are resolved by those commits. I suggest that we hold off making changes here and pick the io.js changes up in the converged repo.

[silkentrance]

Great 👍