Skip to content

Instantly share code, notes, and snippets.

@juliandescottes
Last active October 18, 2019 06:47
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • 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

Re: "using regular events". I guess that for the resource API this would force it to always listen to all target creations & destructions. Might be too much/bad for performance.

@ochameau
Copy link

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?

Note that you could pass only one of the two callbacks if you care about either creation or destruction.

When it comes to the names, I think both onFront and listen should be more explicit, and follow similar naming patterns.

You are very right, I didn't realized it was diverging for probably no good reason.

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.

listen isn't very explicit, and I think it is okish as it is on a specialized TargetList components.
So TargetList.listen, should, by its method and component names convey some useful meaning already.
I still agree on the unecessary differences between listen and onFront.
Unfortunately, listen wouldn't work on Front as this class is full of random stuff, especially if you know that it inherits from Pool!

We could just use regular events

Let me start with this one as I think it highlights the most important point of this API:
onFront and listen fire the creation callback for already existing fronts. So I that I think we should avoid using the notion of "event" and this proposal is a no-go for me because of that.
It will be really hard to fire the callback just for the callback that has been just registered and it looks like a weird behavior to have for an EventEmitter API.

Proposal 2:

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

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

Proposal 3:

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

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

In these two proposals, the word "Event" disturbs me for the same reason.
Proposal 3 makes it worse compared to 2 as it makes "created" be an event, even if it isn't quite an event.
I'm wondering if proposal 2 with onFront and onTarget would work?

Proposal 1:

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

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

This proposal is somewhat ok, but doesn't solve your concerns (which I share) about discrepancies between Front and TargetList namings.

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

Proposal 4 and Proposal 2 (with onFront and onTarget) are, for me, the two competitors.
It is really about how verbose we want to be at the end. I'm not sure we will have a case where we want to listen for destruction only, but I can easily see some code only listening for creation.
Otherwise, both these proposals imply using on, which convey a meaning of event, even if this is slightly more special than events regarding creation event. Using listen was a way to somewhat try to suggest it isn't quite events. This API, for creation is listing and listening at the same time.

What about using:

  • watch/unwatch prefix instead of on to stay away from EventEmitter?
  • listen/unlisten prefix instead of on, for the same reason, but I'm not sure it is any better?
  • (something else you have in mind) prefix instead of on?
  • plural for Fronts and Targets?
  • Available instead of Created to better convey that it is also about existing ones?
  • onTarget/offTarget and onTargetDestroyed (+same for fronts), for the same reason?

@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