Skip to main content
3 of 3
added 260 characters in body
sineemore
  • 1.8k
  • 12
  • 33

Caveats of {}

In JavaScript {} is often used as a map with user provided values being object property names. This can be a recipe for disaster. See this particular piece of code, that results in error:

var ev = new Emitter();

ev.once("toString", function () {
    console.log("Oops!");
});

ev.emit("toString");

The error we'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.

When addHandler is called with event "toString" it checks if "toString" in this.eventHandlers. It finds one in Object.prototype since {} inherits from it. From now on we are in trouble.

If we replace the in with hasOwnProperty call we'll still be in trouble when someone desides 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");

After that all this.eventHandlers.hasOwnProperty expressions will evaluate to array of handlers:

var a = { hasOwnProperty: [] };
a.hasOwnProperty();

To safely call hasOwnProperty on object with user provided property names we should use function from Object.prototype directly:

// JavaScript best practices, so sweet
Object.prototype.hasOwnProperty.call(this.eventHandlers, event);

Actually same stands for any standard method we want to call.

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 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);

You are comparing functions by their string representation:

callback.toString() == this.eventHandlers[event][index].callback.toString()

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 point in if (handler.hasOwnProperty('once')), consider if (handler.once) instead.
  • index is a long name for a loop variable, how about plain old i?
  • for (var i in arr) form is for iteration over object property names where iteration order is not guranteed. To iterate over arrays, use for (var i = 0; i < arr.length; i++) instead.
sineemore
  • 1.8k
  • 12
  • 33