{}
In JavaScript {} is not a pure empty map, so one can't use it in a manner of lookupObj[key] and stay safe.
To overcome this you've used hasOwnProperty method, but check this one:
var ev = new Emitter();
ev.once("toString", function () {
console.log("Oops!");
});
ev.emit("toString");
The error you'll get comes from addHandler function that uses in operator.
The in operator returns true if the specified property is in the specified object or its prototype chain.
If you replace the in with hasOwnProperty call you'll still be in trouble when someone will deside that hasOwnProperty is a good name for an event:
var ev = new Emitter();
ev.once("hasOwnProperty", function () {
console.log("Still in trouble!");
});
ev.emit("hasOwnProperty");
All your this.eventHandlers.hasOwnProperty expressions will evaluate to array of handlers. See this:
var a = { hasOwnProperty: [] };
a.hasOwnProperty();
To safely call hasOwnProperty on object with user provided property names you can use function from Object.prototype directly:
// JavaScript best practices, so sweet
Object.prototype.hasOwnProperty.call(this.eventHandlers, event);
Instead of this I suggest you to use Object.create(null). It will create an empty object without prototype so it will be safe to use bracket lookup notation evading all hasOwnProperty mess.
See it in action:
var a = {};
console.log("a = {}");
console.log("constructor" in a, "toString" in a, "hasOwnProperty" in a);
var a = Object.create(null);
console.log("a = Object.create(null)");
console.log("constructor" in a, "toString" in a, "hasOwnProperty" in a);
See more about this on StackOverflow answer.
once
once method has one particular fail case when the same event is emitted directly from the event handler:
var ev = new Emitter();
ev.once("foo", function () {
console.log("Hey, I was called!");
ev.emit("foo");
});
ev.emit("foo");
I suggest you to remove the once handler right before it being called.
off
The off method removes registered event handler. But what if we call it directly from an event handler:
var ev = new Emitter();
ev.on("foo", function () {
console.log("First");
ev.off("foo");
});
ev.on("foo", function () {
console.log("Second");
});
ev.emit("foo");
Here we have two handlers for the same event "foo".
When the first handler is called it removes all "foo" handlers with delete this.eventHandlers[event].
But when it returns we'll still be in the very for loop trying to access next event handler in this.eventHandlers[event] which was recently deleted:
var foo = { bar: [1, 2] };
for (var i = 0; i < foo.bar.length; i++) {
console.log(foo.bar[i]);
delete foo.bar;
}
Callbacks comparsion
The off method allows the following:
ev.on("foo", function() { /* handler code */ });
ev.off("foo", function() { /* handler code */ });
The right way to remove event handler involves storing the handler function and using it later with off method:
var handler = function() { /* handler code */ };
ev.on("foo", handler);
ev.off("foo", handler);
callback.toString() == this.eventHandlers[event][index].callback.toString()
You are comparing functions by their string representation. Actually, you can compare functions directly:
callback === this.eventHandlers[event][index].callback
Example:
var a = function() {
console.log(Math.PI);
}
var b = function() {
console.log(Math.PI);
}
var c = a;
console.log(function(){} !== function(){}); // different functions
console.log(a !== b); // different functions
console.log(a === c); // same function
console.log(a.toString() === b.toString()); // different functions,
// but same body
Overall behaviour
A quote from Node.js Events documentation:
Note that once an event has been emitted, all listeners attached to it at the time of emitting will be called in order. This implies that any removeListener() or removeAllListeners() calls after emitting and before the last listener finishes execution will not remove them from emit() in progress.
So in short nothing should mutate a pending array of triggered event handlers.
To accomplish this you can slice this.eventHandlers[event] and iterate on it.
let handlers = this.eventHandlers[event].slice(0);
for (var i = 0; i < handlers.length; i++) {
const handler = handlers[i];
// ...
}
This will handle the off error described above.
Other notes
- I don't see any point in
if (handler.hasOwnProperty('once')), considerif (handler.once)instead. indexis a long name for a loop variable, how about plain oldi?for (var i in arr)form is for iteration over object property names where iteration order is not guranteed. To iterate over arrays, usefor (var i = 0; i < arr.length; i++)instead.