Inconsistent Use of Promises

[robmoffat]

Channel contains this, promised API call:

public addContextListener(contextType: string | null, handler: ContextHandler): Promise<Listener>;

PrivateChannel however, has unpromised API calls, which also return listeners:

interface  PrivateChannel extends Channel {
  // methods
  onAddContextListener(handler: (contextType?: string) => void): Listener;
  onUnsubscribe(handler: (contextType?: string) => void): Listener;
  onDisconnect(handler: () => void): Listener;
  disconnect(): void;
}

... which lead to race conditions in my tests, sadly. I doubt we'll ever fix this but raising nonetheless.

[kriswest]

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

[kriswest]

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!

[Roaders]

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

[robmoffat]

@Roaders Agreed but I think @kriswest already included that, no?

[kriswest]

@Roaders @robmoffat I did include it yes - the return type just changes from void to Promise<void>.

[Roaders]

ok, apologies. I must have missed that one.