Return to Answer

Remove context from my implementation, revise recommendation.

Source Link

edited Oct 3, 2018 at 2:04

3.5k
1
13
25

Make up your mind about what version of JS you are targeting.You use let, which was introduced in ES2015, but opt for Array.from(arguments).slice(1) instead of the more readable ...args which was also introduced in ES2015. I recommend going for a later version as it makes it possible to write more readable code.
Since you store events in an object, users are forced to use string keys. This is probably what they will do anyways, but since the specification does not provide this assurance, your implementation should correctly handle more complex event keys. I recommend using a Map instead of Object.create(null).
The specification mentions removing all event handlers. I took this to mean all event handlers, not just all event handlers registered for the passed event. If I call emitter.off(), I would expect the eventHandlers object to be reset.
Don't bother checking if an array contains content before iterating over it. It just creates more noise and provides next to no performance benefit.
Most event emitter libraries allow passing in a context argument to the on which this will be bound to. I'd recommend adding~~Most event emitter libraries allow passing in a context argument to the on which this will be bound to. I'd recommend adding this simple feature as it can greatly improve usability.~~ With arrow functions, I don't use this simple feature asin any implementation I consume, and I don't include it can greatly improve usabilityanymore in implementations I write.
If I pass the same function to on multiple times, what should happen? Should it drop the second subscription? What if the first subscription is with once and the second with on? The reverse? What your library does should be documented and is worth some thought.
Consider failing early if passed invalid arguments. I'd much rather be notified that a handler is not a function when calling on than when calling emit.
Don't repeat yourself! emit can and should use off to remove once events instead of doing it within the function.

// The assert* functions really belong in their own module.

/**
 * Throws an error if fn is not a function.
 * @param {function?} fn
 * @param {string} name
 */
function assertFunction(fn, name) {
  if (typeof fn !== 'function') {
    throw new Error(`Expected ${name} to be a function. Got ${typeof fn}.`);
  }
}

/**
 * Throws an error if arg is not defined.
 * @param {*} arg
 * @param {string} name
 */
function assertDefined(arg, name) {
  if (arg === undefined) {
    throw new Error(`Expected ${name} to be defined.`);
  }
}

/**
 * Factory function to create an event emitter.
 */
function Emitter() {
  const listeners = new Map();
  const getHandlers = event => listeners.get(event) || listeners.set(event, []).get(event);

  /**
   * Attaches a listener to the emitter.
   * If on is called multiple times with the same callback, the callback will be subscribed multiple times.
   * @param {*} event may not be undefined.
   * @param {function} callback
   * @param {*} context this context toonce callif thethis functioncallback withshould whenonly eventsbe arecalled emittedonce.
   */
  function on(event, callback, contextonce = false) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: false });
  }

  /**
   * Attaches a listener to the emitter that will be called at most once.
   * @see on
   * @param {*} event may not be undefined
   * @param {function} callback
   * @param {*} context this context to call the function with when events are emitted.
   */
  function once(event, callback, context) {
    assertDefinedon(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: true });
  }

  /**
   * Removes listeners from an event, or from all events.
   * If a callback is subscribed multiple times, all subscriptions will be cancelled.
   * @param {*} event the event to remove listeners from, if not specified, all listeners on all events will be removed.
   * @param {function?} callback the listener to remove, if not specified all listeners on event will be removed.
   */
  function off(event, callback) {
    if (event === undefined) {
      listeners.clear();
    } else if (callback === undefined) {
      listeners.delete(event);
    } else {
      const handlers = getHandlers(event).filter(handler({ callback: cb }) => handler.callbackcb !== callback);
      listeners.set(event, handlers);
    }
  }

  /**
   * Fires an event, calling all subscribed listeners in their subscription order.
   * If a listener throws, listeners subscribed after that listener will not be called and this function will throw.
   * @param {*} event
   * @param {*[]} args arguments to pass into the subscribed listeners.
   */
  function emit(event, ...args) {
    assertDefined(event, 'event');
    getHandlers(event).slice().forEach(handler => {
      if (handler.once) {
        off(event, handler.callback);
      }
      handler.callback.apply(handler.context, ..args);
    });
  }

  return { on, once, off, emit }
}

const emitter = Emitter()
emitter.on('test', console.log)
emitter.once('test', msg => console.log('>Once<', msg))
emitter.emit('test', 'It works!')
emitter.emit('test', 'Again!')

Make up your mind about what version of JS you are targeting.You use let, which was introduced in ES2015, but opt for Array.from(arguments).slice(1) instead of the more readable ...args which was also introduced in ES2015. I recommend going for a later version as it makes it possible to write more readable code.
Since you store events in an object, users are forced to use string keys. This is probably what they will do anyways, but since the specification does not provide this assurance, your implementation should correctly handle more complex event keys. I recommend using a Map instead of Object.create(null).
The specification mentions removing all event handlers. I took this to mean all event handlers, not just all event handlers registered for the passed event. If I call emitter.off(), I would expect the eventHandlers object to be reset.
Don't bother checking if an array contains content before iterating over it. It just creates more noise and provides next to no performance benefit.
Most event emitter libraries allow passing in a context argument to the on which this will be bound to. I'd recommend adding this simple feature as it can greatly improve usability.
If I pass the same function to on multiple times, what should happen? Should it drop the second subscription? What if the first subscription is with once and the second with on? The reverse? What your library does should be documented and is worth some thought.
Consider failing early if passed invalid arguments. I'd much rather be notified that a handler is not a function when calling on than when calling emit.
Don't repeat yourself! emit can and should use off to remove once events instead of doing it within the function.

// The assert* functions really belong in their own module.

/**
 * Throws an error if fn is not a function.
 * @param {function?} fn
 * @param {string} name
 */
function assertFunction(fn, name) {
  if (typeof fn !== 'function') {
    throw new Error(`Expected ${name} to be a function. Got ${typeof fn}.`);
  }
}

/**
 * Throws an error if arg is not defined.
 * @param {*} arg
 * @param {string} name
 */
function assertDefined(arg, name) {
  if (arg === undefined) {
    throw new Error(`Expected ${name} to be defined.`);
  }
}

/**
 * Factory function to create an event emitter.
 */
function Emitter() {
  const listeners = new Map();
  const getHandlers = event => listeners.get(event) || listeners.set(event, []).get(event);

  /**
   * Attaches a listener to the emitter.
   * If on is called multiple times with the same callback, the callback will be subscribed multiple times.
   * @param {*} event may not be undefined.
   * @param {function} callback
   * @param {*} context this context to call the function with when events are emitted.
   */
  function on(event, callback, context) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: false });
  }

  /**
   * Attaches a listener to the emitter that will be called at most once.
   * @see on
   * @param {*} event may not be undefined
   * @param {function} callback
   * @param {*} context this context to call the function with when events are emitted.
   */
  function once(event, callback, context) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: true });
  }

  /**
   * Removes listeners from an event, or from all events.
   * If a callback is subscribed multiple times, all subscriptions will be cancelled.
   * @param {*} event the event to remove listeners from, if not specified, all listeners on all events will be removed.
   * @param {function?} callback the listener to remove, if not specified all listeners on event will be removed.
   */
  function off(event, callback) {
    if (event === undefined) {
      listeners.clear();
    } else if (callback === undefined) {
      listeners.delete(event);
    } else {
      const handlers = getHandlers(event).filter(handler => handler.callback !== callback);
      listeners.set(event, handlers);
    }
  }

  /**
   * Fires an event, calling all subscribed listeners in their subscription order.
   * If a listener throws, listeners subscribed after that listener will not be called and this function will throw.
   * @param {*} event
   * @param {*[]} args arguments to pass into the subscribed listeners.
   */
  function emit(event, ...args) {
    assertDefined(event, 'event');
    getHandlers(event).slice().forEach(handler => {
      if (handler.once) {
        off(event, handler.callback);
      }
      handler.callback.apply(handler.context, args);
    });
  }

  return { on, once, off, emit }
}

const emitter = Emitter()
emitter.on('test', console.log)
emitter.once('test', msg => console.log('>Once<', msg))
emitter.emit('test', 'It works!')
emitter.emit('test', 'Again!')

Make up your mind about what version of JS you are targeting.You use let, which was introduced in ES2015, but opt for Array.from(arguments).slice(1) instead of the more readable ...args which was also introduced in ES2015. I recommend going for a later version as it makes it possible to write more readable code.
Since you store events in an object, users are forced to use string keys. This is probably what they will do anyways, but since the specification does not provide this assurance, your implementation should correctly handle more complex event keys. I recommend using a Map instead of Object.create(null).
The specification mentions removing all event handlers. I took this to mean all event handlers, not just all event handlers registered for the passed event. If I call emitter.off(), I would expect the eventHandlers object to be reset.
Don't bother checking if an array contains content before iterating over it. It just creates more noise and provides next to no performance benefit.
~~Most event emitter libraries allow passing in a context argument to the on which this will be bound to. I'd recommend adding this simple feature as it can greatly improve usability.~~ With arrow functions, I don't use this feature in any implementation I consume, and I don't include it anymore in implementations I write.
If I pass the same function to on multiple times, what should happen? Should it drop the second subscription? What if the first subscription is with once and the second with on? The reverse? What your library does should be documented and is worth some thought.
Consider failing early if passed invalid arguments. I'd much rather be notified that a handler is not a function when calling on than when calling emit.
Don't repeat yourself! emit can and should use off to remove once events instead of doing it within the function.

// The assert* functions really belong in their own module.

/**
 * Throws an error if fn is not a function.
 * @param {function?} fn
 * @param {string} name
 */
function assertFunction(fn, name) {
  if (typeof fn !== 'function') {
    throw new Error(`Expected ${name} to be a function. Got ${typeof fn}.`);
  }
}

/**
 * Throws an error if arg is not defined.
 * @param {*} arg
 * @param {string} name
 */
function assertDefined(arg, name) {
  if (arg === undefined) {
    throw new Error(`Expected ${name} to be defined.`);
  }
}

/**
 * Factory function to create an event emitter.
 */
function Emitter() {
  const listeners = new Map();
  const getHandlers = event => listeners.get(event) || listeners.set(event, []).get(event);

  /**
   * Attaches a listener to the emitter.
   * If on is called multiple times with the same callback, the callback will be subscribed multiple times.
   * @param {*} event may not be undefined.
   * @param {function} callback
   * @param {*} once if this callback should only be called once.
   */
  function on(event, callback, once = false) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, once });
  }

  /**
   * Attaches a listener to the emitter that will be called at most once.
   * @see on
   * @param {*} event may not be undefined
   * @param {function} callback
   */
  function once(event, callback) {
    on(event, callback, true);
  }

  /**
   * Removes listeners from an event, or from all events.
   * If a callback is subscribed multiple times, all subscriptions will be cancelled.
   * @param {*} event the event to remove listeners from, if not specified, all listeners on all events will be removed.
   * @param {function?} callback the listener to remove, if not specified all listeners on event will be removed.
   */
  function off(event, callback) {
    if (event === undefined) {
      listeners.clear();
    } else if (callback === undefined) {
      listeners.delete(event);
    } else {
      const handlers = getHandlers(event).filter(({ callback: cb }) => cb !== callback);
      listeners.set(event, handlers);
    }
  }

  /**
   * Fires an event, calling all subscribed listeners in their subscription order.
   * If a listener throws, listeners subscribed after that listener will not be called and this function will throw.
   * @param {*} event
   * @param {*[]} args arguments to pass into the subscribed listeners.
   */
  function emit(event, ...args) {
    assertDefined(event, 'event');
    getHandlers(event).slice().forEach(handler => {
      if (handler.once) {
        off(event, handler.callback);
      }
      handler.callback(...args);
    });
  }

  return { on, once, off, emit }
}

const emitter = Emitter()
emitter.on('test', console.log)
emitter.once('test', msg => console.log('>Once<', msg))
emitter.emit('test', 'It works!')
emitter.emit('test', 'Again!')

Source Link

answered Aug 12, 2018 at 0:47

Gerrit0

3.5k
1
13
25

sineemore already covered almost everything I wanted to say.

Make up your mind about what version of JS you are targeting.You use let, which was introduced in ES2015, but opt for Array.from(arguments).slice(1) instead of the more readable ...args which was also introduced in ES2015. I recommend going for a later version as it makes it possible to write more readable code.
Since you store events in an object, users are forced to use string keys. This is probably what they will do anyways, but since the specification does not provide this assurance, your implementation should correctly handle more complex event keys. I recommend using a Map instead of Object.create(null).
The specification mentions removing all event handlers. I took this to mean all event handlers, not just all event handlers registered for the passed event. If I call emitter.off(), I would expect the eventHandlers object to be reset.
Don't bother checking if an array contains content before iterating over it. It just creates more noise and provides next to no performance benefit.
Most event emitter libraries allow passing in a context argument to the on which this will be bound to. I'd recommend adding this simple feature as it can greatly improve usability.
If I pass the same function to on multiple times, what should happen? Should it drop the second subscription? What if the first subscription is with once and the second with on? The reverse? What your library does should be documented and is worth some thought.
Consider failing early if passed invalid arguments. I'd much rather be notified that a handler is not a function when calling on than when calling emit.
Don't repeat yourself! emit can and should use off to remove once events instead of doing it within the function.

I wanted to give this a shot myself, so I added a few tests to address sineemore's points and my own.

it('Works with toString', function() {
  const callback = sinon.fake();
  emitter.on('toString', callback);
  emitter.emit('toString');
  callback.should.have.been.called;
})

it('Works with object references', function() {
  const callback = sinon.fake();
  emitter.on({}, callback);
  emitter.emit({});
  callback.should.not.have.been.called;
});

it('Does not apply `off` calls in the middle of emitting', function() {
  const callback = sinon.fake();
  emitter.once('event', () => {
    emitter.off('event', callback);
  })
  emitter.on('event', callback);
  emitter.emit('event');
  callback.should.have.been.called;
});

Implementation:

// The assert* functions really belong in their own module.

/**
 * Throws an error if fn is not a function.
 * @param {function?} fn
 * @param {string} name
 */
function assertFunction(fn, name) {
  if (typeof fn !== 'function') {
    throw new Error(`Expected ${name} to be a function. Got ${typeof fn}.`);
  }
}

/**
 * Throws an error if arg is not defined.
 * @param {*} arg
 * @param {string} name
 */
function assertDefined(arg, name) {
  if (arg === undefined) {
    throw new Error(`Expected ${name} to be defined.`);
  }
}

/**
 * Factory function to create an event emitter.
 */
function Emitter() {
  const listeners = new Map();
  const getHandlers = event => listeners.get(event) || listeners.set(event, []).get(event);

  /**
   * Attaches a listener to the emitter.
   * If on is called multiple times with the same callback, the callback will be subscribed multiple times.
   * @param {*} event may not be undefined.
   * @param {function} callback
   * @param {*} context this context to call the function with when events are emitted.
   */
  function on(event, callback, context) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: false });
  }

  /**
   * Attaches a listener to the emitter that will be called at most once.
   * @see on
   * @param {*} event may not be undefined
   * @param {function} callback
   * @param {*} context this context to call the function with when events are emitted.
   */
  function once(event, callback, context) {
    assertDefined(event, 'event');
    assertFunction(callback, 'callback');
    const handlers = getHandlers(event);
    handlers.push({ callback, context, once: true });
  }

  /**
   * Removes listeners from an event, or from all events.
   * If a callback is subscribed multiple times, all subscriptions will be cancelled.
   * @param {*} event the event to remove listeners from, if not specified, all listeners on all events will be removed.
   * @param {function?} callback the listener to remove, if not specified all listeners on event will be removed.
   */
  function off(event, callback) {
    if (event === undefined) {
      listeners.clear();
    } else if (callback === undefined) {
      listeners.delete(event);
    } else {
      const handlers = getHandlers(event).filter(handler => handler.callback !== callback);
      listeners.set(event, handlers);
    }
  }

  /**
   * Fires an event, calling all subscribed listeners in their subscription order.
   * If a listener throws, listeners subscribed after that listener will not be called and this function will throw.
   * @param {*} event
   * @param {*[]} args arguments to pass into the subscribed listeners.
   */
  function emit(event, ...args) {
    assertDefined(event, 'event');
    getHandlers(event).slice().forEach(handler => {
      if (handler.once) {
        off(event, handler.callback);
      }
      handler.callback.apply(handler.context, args);
    });
  }

  return { on, once, off, emit }
}

const emitter = Emitter()
emitter.on('test', console.log)
emitter.once('test', msg => console.log('>Once<', msg))
emitter.emit('test', 'It works!')
emitter.emit('test', 'Again!')