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

Commit

Permalink
events: add type checks to once
Browse files Browse the repository at this point in the history 
Also cleanup unnecessary use of "self" since it will always be called
using .apply() from emit.
  • Loading branch information
@trevnorris @isaacs
trevnorris authored and isaacs committed Mar 2, 2013
1 parent e1ac2ef commit d1b4dcd
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions lib/events.js
View file
Expand Up @@ -165,18 +165,18 @@ EventEmitter.prototype.addListener = function(type, listener) {
EventEmitter.prototype.on = EventEmitter.prototype.addListener;

EventEmitter.prototype.once = function(type, listener) {
if ('function' !== typeof listener) {
throw TypeError('.once only takes instances of Function');
}
if (typeof type !== 'string')
throw TypeError('type must be a string');
if (typeof listener !== 'function')
throw TypeError('listener must be a function');

var self = this;
function g() {
self.removeListener(type, g);
this.removeListener(type, g);
listener.apply(this, arguments);
};
}

g.listener = listener;
self.on(type, g);
this.on(type, g);

return this;
};
Expand Down

4 comments on commit d1b4dcd

@balupton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're looking into a strange bug - docpad/docpad#462 - where we are getting TypeError: Object #<Object> has no method 'removeListener' which points to this this.removeListener call - https://github.com/joyent/node/blob/v0.10.1/lib/events.js#L174

Could this removal of self for this be the cause of it? Will be looking into it further, grateful for any feedback you'll be able to provide :)

@bnoordhuis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible but it suggests you're calling the EventEmitter object in a non-EventEmitter context, i.e. something like ee.emit.call({}, 'event').

@balupton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we extend the EventEmitter class to support synchronous and asynchronous events with a completion callback once they're all completed. We've made it so our extension does the bind in the same way 0.10 does it, which fixes the problem - it does feel more like a work around, but it does the job - if there was some documentation about event listeners scope in the api docs that would be cool, as it would feel less like a bug and more like a spec then - I'm not fussed either way, as our issue is now resolved.

For the background, here's the code we use - https://github.com/balupton/bal-util/blob/v1.16.13/src/lib/events.coffee#L8-L47 - and the relevant tests - https://github.com/balupton/bal-util/blob/v1.16.13/src/test/events.test.coffee#L8-L106

@bnoordhuis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there was some documentation about event listeners scope in the api docs

Assume normal JS scoping rules unless explicitly documented otherwise (that is, this should be a valid instance of the class whose method you're invoking).

It's unfortunate that it broke for you in v0.10 but the fact that it worked in v0.8 was purely by accident, an implementation detail if you want.

Please sign in to comment.