Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrow fetchMessages response type using includeMessageActions param #408

Closed
yo1dog opened this issue Oct 14, 2024 · 3 comments
Closed

Narrow fetchMessages response type using includeMessageActions param #408

yo1dog opened this issue Oct 14, 2024 · 3 comments
Labels
priority: low This PR should be reviewed after all high and medium PRs. status: rejected This issue is considered rejected. It will not be worked on. type: chore This PR contains changes that are not covered by other types (stylistic, dependency updates, etc).

Comments

@yo1dog
Copy link

yo1dog commented Oct 14, 2024

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 parfeon added status: rejected This issue is considered rejected. It will not be worked on. priority: low This PR should be reviewed after all high and medium PRs. type: chore This PR contains changes that are not covered by other types (stylistic, dependency updates, etc). labels Oct 30, 2024
@parfeon
Copy link
Contributor

parfeon commented Oct 30, 2024

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

@parfeon parfeon closed this as completed Oct 30, 2024
@yo1dog
Copy link
Author

yo1dog commented Oct 31, 2024

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
Copy link
Contributor

parfeon commented Oct 31, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low This PR should be reviewed after all high and medium PRs. status: rejected This issue is considered rejected. It will not be worked on. type: chore This PR contains changes that are not covered by other types (stylistic, dependency updates, etc).
Projects
None yet
Development

No branches or pull requests

2 participants