Skip to content

Instantly share code, notes, and snippets.

@juliandescottes
Last active April 26, 2024 22:53
Show Gist options
  • Save juliandescottes/835fc7e71cbaac1c223ee0e7c6e8d1b3 to your computer and use it in GitHub Desktop.
Save juliandescottes/835fc7e71cbaac1c223ee0e7c6e8d1b3 to your computer and use it in GitHub Desktop.

API consistency between Front and TargetList

Fronts have onFront(type, callback), offFront(type, callback). The TargetList has listen(type, createCallback, destroyCallback) and unlisten(type, createCallback, destroyCallback).

The two APIs seem to care about similar lifecycles, so it would be great if the API had a similar shape. Having a single method with 2 callbacks means all consumers need to attach their listeners for creation and destruction at the same spot. It might not be flexible enough in all situations?

Proposal 1:

// Front
onFront(type, createCallback, destroyCallback);
offFront(type, createCallback, destroyCallback);

// TargetList
listen(type, createCallback, destroyCallback);
unlisten(type, createCallback, destroyCallback);

Self explanatory method names

When it comes to the names, I think both onFront and listen should be more explicit, and follow similar naming patterns. Front::onFront may be confusing because "front" is not an event, it's not something that happens. TargetList::listen is very generic.

Something like Front::onFrontEvent/TargetList::onTargetEvent, either with 2 callbacks.

Proposal 2:

// Front
onFrontEvent(type, createCallback, destroyCallback);
offFrontEvent(type, createCallback, destroyCallback);

// TargetList
onTargetEvent(type, createCallback, destroyCallback);
offTargetEvent(type, createCallback, destroyCallback);

Or instead of the double callback, an additional "eventName" argument which would be either created or destroyed. In that way it would be more similar to a classic event listening API.

Proposal 3:

// Front
onFrontEvent(type, eventName, callback);
offFrontEvent(type, eventName, callback);

// TargetList
onTargetEvent(type, eventName, callback);
offTargetEvent(type, eventName, callback);

Or dedicated methods for each:

Proposal 4:

// Front
onFrontCreated(type, callback);
offFrontCreated(type, callback);

onFrontDestroyed(type, callback);
offFrontDestroyed(type, callback);

// TargetList
onTargetCreated(type, callback);
offTargetCreated(type, callback);

onTargetDestroyed(type, callback);
offTargetDestroyed(type, callback);

We could just use regular events

Finally I want to also mention the option of not adding new APIs and only rely on events. This means the type filtering would be done on the consumer instead of doing it in the emitter, but at least, no new API to introduce.

Proposal 5: There is no new API for this proposal, but consumers would look like:

// Front
front.on("front-created", (front, type) => {
  if (type === "type-I-want-watch") {
    doStuffWithFront(front)
  }
});
// ...
targetList.on("target-created", (target, type) => { /* same idea... */ })

Of course this means we need to document the events that are fired by those classes, document the event payload etc... Events are also an API so it's not a magical solution, but since after all those APIs are really about reacting to events, I think reusing our existing event-emitter APIs could make sense.

@juliandescottes
Copy link
Author

Thanks for the feedback @ochameau

Indeed, I forgot again that we had to apply the create callback on all the already available fronts/targets.
And with this in mind, I agree we should find a name that doesn't make consumers think about this as an event :)

I really like your watch/unwatch proposal + using plural. Gives uswatchFronts/unwatchFronts & watchTargets/unwatchTargets. I like it better than listen/unlisten mostly because listenFronts doesn't sound correct and we'd have to use listenToFronts.

  • Available instead of Created

I think that's a very good suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment