pubnub / javascript

PubNub JavaScript SDK docs https://www.pubnub.com/docs/sdks/javascript
Other
553 stars 401 forks source link

Narrow fetchMessages response type using includeMessageActions param #408

Closed yo1dog closed 3 weeks ago

yo1dog commented 1 month ago

Rather than fetchMessages always returning union of FetchMessagesWithActionsResponse and FetchMessagesForChannelsResponse, you can use the value of the includeMessageActions flag in FetchMessagesParameters to narrow the response type like so:

fetchMessages(parameters: History.FetchMessagesParameters & {includeMessageActions: true}): Promise<History.FetchMessagesWithActionsResponse>;
fetchMessages(parameters: History.FetchMessagesParameters & {includeMessageActions?: false}): Promise<History.FetchMessagesForChannelsResponse>;
parfeon commented 3 weeks ago

Great idea, but doesn't work properly with TS functions overloading, which we use in our SDK. TS complains about duplicated functions.

yo1dog commented 3 weeks ago

This works fine in pubnub-common.ts. This is precisely what TypeScript overload signatures was built for:

  /**
   * Fetch messages history for channels.
   *
   * @param parameters - Request configuration parameters.
   *
   * @returns Asynchronous fetch messages response.
   */
  public async fetchMessages(parameters: History.FetchMessagesParameters): Promise<History.FetchMessagesResponse>;
  public async fetchMessages(parameters: History.FetchMessagesParameters & {includeMessageActions: true}): Promise<History.FetchMessagesWithActionsResponse>;
  public async fetchMessages(parameters: History.FetchMessagesParameters & {includeMessageActions?: false}): Promise<History.FetchMessagesForChannelsResponse>;
parfeon commented 3 weeks ago

Those are great if it was just promises, but there is version with callbacks, so there are two overloads to single method which handles both versions: promise and callback. Before writing you respond, I tried it and IDE wasn't happy with my attempts and reported duplicated methods (I was in the middle of other changes, and maybe they affected it).

  public fetchMessages(
    parameters: History.FetchMessagesParameters & { includeMessageActions: true },
    callback: ResultCallback<History.FetchMessagesWithActionsResponse>,
  ): void;

  public fetchMessages(
    parameters: History.FetchMessagesParameters & { includeMessageActions?: false },
    callback: ResultCallback<History.FetchMessagesForChannelsResponse>,
  ): void;

The first method reports overload signature incompatibility. Meantime, there are no issues with async versions:

  public async fetchMessages(
    parameters: History.FetchMessagesParameters & { includeMessageActions: true },
  ): Promise<History.FetchMessagesWithActionsResponse>;

  public async fetchMessages(
    parameters: History.FetchMessagesParameters & { includeMessageActions?: false },
  ): Promise<History.FetchMessagesForChannelsResponse>;