Closed robmoffat closed 1 week ago
Those were supposed to be async. I think they were introduced at the same time as we made the rest of the API async and this probably went in at around the same time or just before and no one noticed the discrepency - I'm suprised nobody's brought it up befor enow. We should fix it for consistency, but it'd require a vote. Alternatively we could deprecate these and introduce some new async versions? Perhaps use the addEventListener
pattern instead so we only need 2 functions (addEventListener and disconnect)?
So to make that a concrete proposal:
we could change:
interface PrivateChannel extends Channel {
// methods
onAddContextListener(handler: (contextType?: string) => void): Listener;
onUnsubscribe(handler: (contextType?: string) => void): Listener;
onDisconnect(handler: () => void): Listener;
disconnect(): void;
}
to:
interface PrivateChannel extends Channel {
addEventListener(type: PrivateChannelEventType | null, handler: EventHandler): Promise<Listener>;
disconnect(): Promise<void>;
// deprecated methods
/** @deprecated use `addEventListener("addContextListener", handler)` instead. */
onAddContextListener(handler: (contextType?: string) => void): Listener;
/** @deprecated use `addEventListener("unsubscribe", handler)` instead. */
onUnsubscribe(handler: (contextType?: string) => void): Listener;
/** @deprecated use `addEventListener("disconnect", handler)` instead. */
onDisconnect(handler: () => void): Listener;
}
export enum PrivateChannelEventType {
ADD_CONTEXT_LISTENER = "addContextListener"
UNSUBSCRIBE = "unsubscribe"
DISCONNECT = "disconnect"
}
2.2 would actually be good time to do that as we're introducing addEventListener
to the DesktopAgent API, fixing a second consistency issue!
PrivateChannel.disconnect()
is probably another candidate for this. It's an async process with a PrivateChannelDisconnectRequest
and PrivateChannelDisconnectResponse
so we should be returning a promise IMHO
@Roaders Agreed but I think @kriswest already included that, no?
@Roaders @robmoffat I did include it yes - the return type just changes from void
to Promise<void>
.
ok, apologies. I must have missed that one.
Channel
contains this, promised API call:PrivateChannel
however, has unpromised API calls, which also return listeners:... which lead to race conditions in my tests, sadly. I doubt we'll ever fix this but raising nonetheless.