Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

EventEmitter breaking changes in latest builds #4971

Closed
billti opened this issue Mar 10, 2013 · 5 comments
Closed

EventEmitter breaking changes in latest builds #4971

billti opened this issue Mar 10, 2013 · 5 comments

Comments

@billti
Copy link

billti commented Mar 10, 2013

With the change in 4f7f8bb to initialize the _events object in the EventEmitter constructor, the behavior has changed notably. This breaks some popular libraries I use (such as 'jake' for building). The issue is that now the _events object is on the EventEmitter instance, not whatever 'this' called addEventListener. Code that uses a common EventEmitter instance as a constructor function prototype get incorrect behavior now, as all constructed object instances share the same list of handlers.

The below simple test works on 0.8.22, and if I build a commit earlier than the above, but fails from that commit on (both added listeners fire for both Task instances, so eventCount is 4). Simply reverting line 35 in ./lib/events.js to initialize _events to null (or even comment out that line entirely) fixes the issue.

var assert = require('assert');
var EventEmitter = require('events').EventEmitter;
var eventCount = 0;

var Task = function(name){
    this.name = name;
    this.complete = function(){this.emit('complete');};
};

Task.prototype = new EventEmitter();
Task.prototype.constructor = EventEmitter;

var t1 = new Task('t1');
var t2 = new Task('t2');
t1.addListener('complete', function() {eventCount++;})
t2.addListener('complete', function() {eventCount++;})

t1.emit('complete');
t2.emit('complete');

assert.equal(eventCount, 2, "Incorrect invocation count for event handlers")
@TooTallNate
Copy link

Well sadly these JavaScript libs that are now breaking are doing inheritance wrong. You don't call new EventEmitter() and set that as the prototype, since that causes the exact problems that you're encountering. The constructor of the class is indeed exactly where instance variables should be initialized, so the change was correct IMO.

These now broken libs should switch over to using util.inherits() or some other proper form of inheritance.

@billti
Copy link
Author

billti commented Mar 10, 2013

Might be worth making a note on https://github.com/joyent/node/wiki/Api-changes-between-v0.8-and-v0.10 if folks could get bitten by it. Took me all day to figure it out. (Partly as node-inspector also appears to be broken on the latest v0.10 - my Node command-line debugger skills aren't quite as brisk).

@isaacs
Copy link

isaacs commented Mar 10, 2013

@isaacs isaacs reopened this Mar 10, 2013
@isaacs isaacs closed this as completed Mar 10, 2013
@billti
Copy link
Author

billti commented Mar 12, 2013

Thanks. Just as a note, but the "Correct Style Inheritance" you demonstrate in the page is actually wrong, as you are not calling the parent constructor from the child constructor. This should be done (as the snippet in the docs at util.inherits actually demonstrates), as if the parent constructor isn't called then any properties it sets won't get created (e.g. such as _events and _maxListeners on EventEmitter). Sample below demonstrates.

Not a biggy, but if the doc is going to make a point about "Correct Style Inheritance", it should probably be correct :-)

var EventEmitter = require('events').EventEmitter;
var util = require('util');

function EventImpl1(){

};
function EventImpl2(){
    EventEmitter.call(this);
};


util.inherits(EventImpl1, EventEmitter);
util.inherits(EventImpl2, EventEmitter);

var e1 = new EventImpl1();
var e2 = new EventImpl2();

console.log(e1._maxListeners);  // undefined (as is _events)
console.log(e2._maxListeners);  // 10

@TooTallNate
Copy link

Fixed.

On Mon, Mar 11, 2013 at 6:55 PM, billti notifications@github.com wrote:

Thanks. Just as a note, but the "Correct Style Inheritance" you
demonstrate in the page is actually wrong, as you are not calling the
parent constructor from the child constructor. This should be done (as the
snippet in the docs at util.inheritshttp://www.nodejs.org/api/util.html#util_util_inherits_constructor_superconstructoractually demonstrates), as if the parent constructor isn't called then any
properties it sets won't get created (e.g. such as _events and
_maxListeners on EventEmitter). Sample below demonstrates.

Not a biggy, but if the doc is going to make a point about "Correct Style
Inheritance", it should probably be correct :-)

var EventEmitter = require('events').
EventEmitter;
var util = require('util');

function EventImpl1(){

};
function EventImpl2(){
EventEmitter.call(this);
};

util.inherits(EventImpl1, EventEmitter);
util.inherits(EventImpl2, EventEmitter);

var e1 = new EventImpl1();
var e2 = new EventImpl2();

console.log(e1._maxListeners); // undefined (as is _events)
console.log(e2._maxListeners); // 10


Reply to this email directly or view it on GitHubhttps://github.com//issues/4971#issuecomment-14754475
.

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

No branches or pull requests

3 participants