Prototype bleeding in events.js

[martinheidegger]

Inheriting from EventEmitter using .prototype-old-school-notation like this:

var MyClass = function() {};
MyClass.prototype = new EventEmitter();

will result in the listeners added to one instance being executed by listeners of another instance.

var ok = new MyClass();
var broken = new MyClass();
broken.on('x', function () { console.log('b'); });
ok.on('x', function () { console.log('a'); });
ok.emit('x'); // b a

This can be quite easily resolved by changing the inheritance to:

util.inherits(MyClass, EventEmitter);

or

MyClass.prototype = Object.create(EventEmitter.prototype);

but it still is a problem because even developers like substack seem to not be aware of this: https://github.com/substack/node-charm/blob/c6646deed2dcee7f512bbbfac6d2ab58fb528a68/index.js#L67

This PR - at the time of writing this - still just contains just the test. I am not sure how to fix this. Any help welcome.

Note: This fixes a problem that exists in older node versions as well, however: I found this bug/problem while investigating a different bug that started to appear in io.js after version 1.3.0 either due to 7061669dba or 630f636334 However my other bug was too hard to properly reproduce. Fixing this will fix the other bug as well.

[vkurchatkin]

This is a regression (I think)

[bnoordhuis]

I think so too. nodejs/node-v0.x-archive#7157 was fixed over a year ago.

[thefourtheye]

This function must be called, right? Why not assign some value to a variable here and check if the value is set in process.exit?

[martinheidegger]

@thefourtheye Thank you for pointing the various things out. I pushed a style correction.

[thefourtheye]

I think we really have nothing to do here. Just clarifying how to properly inherit from EventEmitter in the docs should be enough. #2168

[brendanashworth]

The doc change was landed in d168d01. @thefourtheye is this still up for landing or should it be closed?

[martinheidegger]

https://github.com/search?utf8=%E2%9C%93&q=%22.prototype+%3D+new+EventEmitter%28%29%22&type=Code&ref=searchresults 4000 entries + in github that use that problematic, non-standard use of EventEmitter (word by word). And they are usually submissions of people that are confident enough to submit something to github. If it is only treated as a "documentation fixes this issue" (which I think it doesnt) then it should specifically mention that incompatibility.

Note: Old-school JavaScript inheritance like MyEventEmitter.prototype = new EventEmitter() does not work.

Also I think that it should be more prominent in the docs that this way of inheritance is actively regarded as harmful.

[thefourtheye]

@brendanashworth sorry. I couldn't respond earlier. But we already have this regression here https://github.com/nodejs/node/blob/master/test/parallel/test-event-emitter-subclass.js#L38-L49. So we can close this.

@martinheidegger You may want to propose a documentation change in a separate PR.

[brendanashworth]

It is unfortunate that so many people use this inheritance, but it doesn't make sense in the least. I agree a documentation fix is in order. Closing.