Last week I demonstrated a simple event class implementation. Some readers pointed out weaknesses in that implementation, so I’ve written fixes and regression tests.

Listener Removal

Richard pointed out that listeners that remove themselves might cause unexpected side effects:

Take care when removing listeners! If the listener itself calls remove, all the listeners added after it are moved up one in the array. You may cause one of the elements to not be called. i.e., the element that gets moved into the position of the removed listener.

To put it concisely: removing listeners mutates the same array that’s used to fire events.

If you’re still puzzled, consider this test:

'test listener removal in Emitter': function() {
  var Emitter = turing.events.Emitter,
      emitter = new Emitter(),
      i = 0,
      j = 0;
  
  function add() {
    i++;
    assert.ok(emitter.removeListener('add', add));
  }

  // This should end up being called twice
  function add2() {
    j++;
  }

  emitter.on('add', add);
  emitter.on('add', add2);

  assert.ok(emitter.emit('add'));
  assert.ok(!emitter.removeListener('add', add));
  assert.ok(emitter.emit('add'));
  assert.equal(2, j);
}

There are two counters that get incremented when the same event, 'add', fires: i and j. The add listener removes itself, so it should only fire once. I test this by making sure it can’t be removed after it fires:

assert.ok(!emitter.removeListener('add', add));

Then I fire the same event again, which should still trigger add2. Before fixing this, the test will display an error like this:

✕ Assertion failed in: test listener removal in Emitter
AssertionError: 1 == 2

But how do we fix it? Well, the clue is in my description of the problem: the underlying event listener array is being mutated. The method that calls listeners, emit, needs to work with a copy of the array.

How do we copy an array? An easy way is to use Array.prototype.slice, and this is what Node’s events.js code does.

The fixed code doesn’t look too different from the original:

emit: function(eventName) {
  var fired = false;
  if (eventName in this.events === false) return fired;

  var list = this.events[eventName].slice();

  for (var i = 0; i < list.length; i++) {
    list[i].apply(this, Array.prototype.slice.call(arguments, 1));
    fired = true;
  }
  
  return fired;
}

Deleting Items from Arrays

Cameron Knight had this to say:

For your array remove, you should just use array.splice(i, 1); If you read the comments on Resig’s blog, even he makes this conclusion.

I’ve been avoiding splice() for a long time because I once hit a browser support issue with it. Unfortunately, I’ve forgotten the exact reasons why. I decided to forge ahead and replace the complicated removeListenerAt method:

// Before:
removeListenerAt: function(eventName, i) {
  var array = this.events[eventName],
      rest = array.slice(i + 1);
  array.length = i;
  array.push.apply(array, rest);
  this.events[eventName] = array;
}

// After:
removeListenerAt: function(eventName, i) {
  this.events[eventName].splice(i, 1);
}

I tested this in recent versions of Chrome, Firefox, and IE6 and above, and it worked well. Whatever made me avoid splice seems invalid now.

Conclusion

Thanks for your comments, this event handling code is definitely looking a lot sharper now!

The latest commit was 1a41b80.

Other Recent Posts

• 21 May 2012
  JS101: Prototypes
• 18 May 2012
  Page, testr.js, screenfull.js
• 17 May 2012
  Windows and Node: Addons