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.
Note that you could pass only one of the two callbacks if you care about either creation or destruction.
You are very right, I didn't realized it was diverging for probably no good reason.
listen
isn't very explicit, and I think it is okish as it is on a specializedTargetList
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
andonFront
.Unfortunately,
listen
wouldn't work onFront
as this class is full of random stuff, especially if you know that it inherits fromPool
!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.
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
andonTarget
would work?This proposal is somewhat ok, but doesn't solve your concerns (which I share) about discrepancies between Front and TargetList namings.
Proposal 4 and Proposal 2 (with
onFront
andonTarget
) 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. Usinglisten
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?Fronts
andTargets
?Available
instead ofCreated
to better convey that it is also about existing ones?onTarget
/offTarget
andonTargetDestroyed
(+same for fronts), for the same reason?