Javascript (ES11) Event System implementation

Question

So I decided to implement a basic event system in JavaScript.

My goal was to support the following functions:

on - allows the user to register a handler on the event system that fires until it's removed.

once - same as above, but only fires the first time, then removes itself.

emit - creates an event and fires all relevant handlers.

delete - deletes the specified handler from from the system.

class HandlerSet extends Map {
 set(handler, once = false) {
 if (typeof handler !== 'function') return
 super.set(handler, !once ? handler : (...args) => {
 handler(...args)
 this.delete(handler)
 })
 }
 forEach(...args) {
 [...this.values()].forEach(...args)
 }
}
class EventSystem extends Map {
 set(type) {
 return super.set(type, new HandlerSet())
 }
 on(type, handler, once = false) {
 (this.get(type) ?? this.set(type).get(type)).set(handler, once)
 }
 once(type, handler) {
 this.on(type, handler, true)
 }
 emit(type, ...args) {
 this.get(type)?.forEach(handler => handler(...args))
 }
 delete(type, handler) {
 let set = this.get(type)
 if (set?.delete(handler) && set?.size === 0) {
 super.delete(type)
 }
 }
}

I'm curious to see what peoples thoughts are on this implementation.

As far as I'm aware, it should be quite robust from a type safety standpoint, as well as quite predictable from a lifecycle standpoint.

If anyone can find any bugs in this, or suggest improvements, I'd like to hear from you!

Answer 1

I couldn't find any obvious bugs, so that's good!

For improvements, I have a few comments. It's mostly about readability, and of course it's just my subjective experience when I read the code:

• If handler is not a function, you should throw an error. It's obviously a mistake.
• When extending base classes you should not rewrite the extended method signatures. This would be obvious in typescript, where you would get an error message. The reason is that it would be legal to assign EventSystem to a Map, but that would not work since EventSystem is not a Map (due to the rewrite).
• The way EventSystem is implemented it exposes many more methods than it needs to. Why expose set? Also, it will expose all the other methods on Map, which you may or may not want.
• In HandlerSet, you seem to want a data structure that mimics a Set. The Map is an implementation detail, and it works quite well. However, when you add an item to a regular set the method is called "add". It's just a minor point, but it's small things like this that happens when you extend a base class that doesn't quite fit.
• In the method "on" you have something that reads a bit like "get set get set" with some parens that needs to be mentally arranged. It's harder to read than it needs to be, at least in my opinion. I would split it onto two lines to make it completely obvious.
• HandlerSet.forEach seems a bit strange to me. You've extended Map, so why not use the Map methods directly from EventSystem? Or if not, why not make a function that actually calls the handlers? Extending Map in HandlerSet is completely pointless since you don't use a single method from Map (except delete). The Map should be an implementation detail imo.

• Hex: Good point on passing a non function to the set, and the fact it should throw an error. HandlerSet originally extended a set, however I needed a way to find the handler function within the set even if it was wrapped by a cleanup function as seen when the bool "once" is passed to the set function. When it is wrapped, it can no longer be found using the original function. As far as overriding functions and changing the parameters, as far as I'm aware, this isn't an error in JavaScript, rather I'm just embracing a language feature.
• Hex: I also agree the 'on' function implementation should be adjusted to be a bit more readable. And I also do agree with changing the override of forEach to instead call the functions directly, thanks for the comprehensive review!
• Magnus Jeffs Tovslid: Using a map for handlerset is completely fine, it was just the extending I didn't like. You can call it embracing a language feature if you want, but just because javascript allows you to do all sorts of crazy things doesn't mean it's a good idea. I've outlined some problems, but it's up to you to decide if they're important or not. See also "liskov substitution" principle if you want to know more

Answer 2

Events or messages?

Is this a messaging system or an event system. As I see it, it is a messaging system.

Assuming this is for the browser you may prefer the provided events.Event API which will use the event stack. NOTE check comparability before using.

Review

A general style code review. Code style is subjective and thus these points are just suggestions

• When possible use instanceof rather than typeof as the former has considerably less overhead

  For example

  if (typeof handler === 'function') {
  

  can be

  if (handler instanceof Function) {
  

• Do not silence errors (oh I mean... "NEVER!! silence errors"). The above point (vetting the callback) will silence code that is passing the wrong type of object. Silencing means that an error can go unnoticed during development.

  Either throw when vetting or let JS throw naturally.

• Always delimit code blocks with {} Eg if (foo) return should be if (foo) { return }

• Use semicolons!

• Use constants whenever possible. You have let set = this.get(type) should be const set = ...

• Avoid redundant clauses

  You have the statement

  if (set?.delete(handler) && set?.size === 0) {
  

  The second ?. is redundant. If the first clause is true then set must not be undefined.

  Also use logical not ! rather than === 0. Map.size will only ever be a Number thus !set.size will always be true for 0

  if (set?.delete(handler) && !set.size) {
  

• Avoid intermediate calls, especially when using rest parameters as this forces array creation overhead just to pass arguments between functions.

  You have

  class HandlerSet extends Map {
   forEach(...args) {
   [...this.values()].forEach(...args);
   }
  class EventSystem extends Map {
   emit(type, ...args) {
   this.get(type)?.forEach(handler => handler(...args));
   }
  

  Can be...

  emit(type, ...args) {
   const handlers = this.get(type);
   if (handlers) {
   for (const handler of handlers.values()) { handler(...args) }
   }
  }
  

  ...reducing memory and iteration overheads. Also removes the need for HandlerSet.forEach

• Be careful with naming.

  Avoid naming variables for their type. Name variables for what they represent, not what they are.

  I find your name selection is obfuscating the functionality of your code. The main problem is that the concepts of a listener and an event have been blurred. An event has a name eg a load event would be called "load". Each named event has a set of listeners / callbacks (functions)

  Notes on some of the names you used

  • handler Conventional JS uses listeners rather than handlers, however in this case they are much more like callbacks, listener or callback would be more appropreate (NOTE callback as one word not callBack).

  • HandlerSet is extending a Map. Names should avoid including type descriptions. Adding a name of a similar type is just confusing.

  • EventSystem is it really a system? Don't add superfluous content to names. Maybe GlobalEvents would be better?

  • EventSystem.on ambiguous name, maybe addListener, addCallback, or even just addCall?

  • EventSystem.once Similar to on. Personally the once functionality is inappropriate or can be handled using options when adding a listener.

  • EventSystem.emit JS convention is that events are fired not emitted thus maybe fire would be better

  • EventSystem.delete Delete what, an event or a listener? Names should never be ambiguous.

Rewrite?... No.

I did rewrite your code but I could not reconcile its usefulness due to ambiguity of (guessed) usage cases.

Thus what follows is an Example and is just a modification of code I use to add a messaging.

Example

Note the example code has been parsed but is untested.

Note the Map named events is closed over and thus protected from miss use.

Note This does not check if the listener is a function. Doing so can hide/silence bad code. If the cb is not a function it will throw in fireEvent

// Naming: cb and cbs short for "callback" (a function) and "callbacks" (array of callback objects)
function Events(owner = {}) {
 const events = new Map(); // named arrays of listener objects
 return Object.assign(owner, {
 addListener(name, cb, options = {}) {
 var cbs;
 ((events.get(name) ?? (events.set(name, cbs = []), cbs)).push({cb, options}));
 },
 removeEvent(name) { events.delete(name) },
 removeListener(name, cb) {
 const cbs = events.get(name);
 if (cbs?.length) {
 const idx = cbs.findIdx(listener => listener.cb === cb);
 idx > -1 && cbs.splice(idx, 1);
 }
 },
 fireEvent(name, data = {}) {
 var i = 0;
 const cbs = events.get(name);
 if (cbs?.length) { 
 for (const {cb, options} of cbs) { 
 options.once && cbs.splice(i--, 1);
 cb.bind(owner)({type: name, target: owner, ...data});
 i++;
 } 
 }
 },
 });
}
export {Events};