@slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype #1670

filmaj · 2023-10-04T20:44:26Z

⚠️ DO NOT MERGE ⚠️

Purely Intended For Discussion

This draft PR prototypes addressing #1323: the web API arguments and responses not being type-safe due to allowing arbitrary properties in both request and response payloads.

This passes tests locally, and I also integrated with bolt-js, @slack/oauth and @slack/socket-mode locally (via npm link) and the local unit tests in those packages also passed.

TL;DR: should we do more fine-grained testing against production of this change to ensure this doesn't break anything?

I think manual testing might be needed before merging this change. There are many different HTTP API methods with a variety of arguments and responses. I think it may be worth testing every method to see if at least basic interactions with each API are possible. 😬 Not sure, what do you think?

On the flip side, if I am able to put the time into manual testing each method, I think there would be many benefits:

A good ol' audit and opportunity to add better documentation and make sure things look relevant.
Could carefully examine cases where certain APIs accept either/or style of arguments. For example, the chat.postMessage API requires one of attachments, blocks or text. Currently we model these as optional properties, instead, we should model them as unions. I included a prototype of this in this PR - jump to this comment of mine to see what that looks like in src/method.ts.
a. Could also do this for API response shapes.
b. This could also be left as an exercise for the future. For example, maybe such an audit, and modifying the request and response payloads for Slack's HTTP API, could be left as something to work on for an eventual web-api 8.0 release.
c. I am also considering moving these kinds of shapes to the @slack/types repo. Let me know if you have thoughts on this (I posted about this in our internal Slack team channel as well). Pulling the types out of this repo would ease re-use in other projects, such as the deno SDK projects.

Depending on the discussion, I could see different potential outcomes / decisions and I am looking for feedback from the team on these:

We accept just the removal of arbitrary properties from API request/responses, wanting to move forward with addressing only WebClient arguments and responses are not type safe in TypeScript #1323 in the upcoming 7.0 release.
Regarding more finely typing the request/response shapes, not worth it. Or maybe it is, but not right now. Or maybe it is and we want to do this for the impending 7.0 release. You tell me what you think!
You like the idea of adding request/response shapes to @slack/types and having @slack/web-api pull them from there. Or maybe you don't and you think we should keep them here.

…eaning up the readme. remove no longer used codecov and deno build npm run scripts. update dependencies as much as possible. fix linter errors. remove a test that is no longer applicable with new axios.

…g, unknown>`. Remove unneeded disabling of no-trailing-space rule. Replace use of `headers: any` in private `serializeApiCallOptions` method with `Record<string, string>`.

…allResult. Tweak fileUploadV2 return type as a result.

…lines up behaviour with WebClient.

…guments.

filmaj · 2023-10-04T20:50:12Z

packages/web-api/src/WebClient.ts

@@ -64,10 +64,6 @@ export enum WebClientEvent {
  RATE_LIMITED = 'rate_limited',
 }

-export interface WebAPICallOptions {


Removing this generic interface, which is the basis for all API request arguments. This is the key change for addressing #1323 .

This is great! but to proceed with this, we may need to review all the argument types to verify if they have latest full list. Have you already done?

I have not, and wanted to get feedback on this point; I mentioned in my original post that this change is significant enough that maybe it requires testing all the endpoints. It sounds to me like you agree that testing is needed. So, I will proceed with that.

filmaj · 2023-10-04T20:50:56Z

packages/web-api/src/WebClient.ts

@@ -82,7 +78,6 @@ export interface WebAPICallResult {
    // `chat.postMessage` returns an array of error messages (e.g., "messages": ["[ERROR] invalid_keys"])
    messages?: string[];
  };
-  [key: string]: unknown;


The other key change to address #1323: request responses allow for arbitrary keys, making them type-unsafe.

If the currently generated response types miss some of response properties, this change may affect TypeScript users. That being said, we can improve it by providing complete typing as much as possible.

filmaj · 2023-10-04T20:53:21Z

packages/web-api/src/WebClient.ts

   * @param method - the Web API method to call {@link https://api.slack.com/methods}
   * @param options - options
   */
-  public async apiCall(method: string, options: WebAPICallOptions = {}): Promise<WebAPICallResult> {


Any areas where we still want things to extend from a shapeless foundation, we can instead use Record<string, unknown> or Record<string, any> to communicate "an object with any shape but with root keys being strings".

yeah, i don't see any issues with this change

filmaj · 2023-10-04T20:53:55Z

packages/web-api/src/WebClient.ts

   * **#4**: Unless `request_file_info` set to false, call {@link https://api.slack.com/methods/files.info files.info} for
   * each file uploaded and returns that data. Requires that your app have `files:read` scope.
   * @param options
   */
-  public async filesUploadV2(options: FilesUploadV2Arguments): Promise<WebAPICallResult> {
+  public async filesUploadV2(options: FilesUploadV2Arguments): Promise<
+  WebAPICallResult & { files: FilesCompleteUploadExternalResponse[] | WebAPICallResult[] }


Small change to make this part typesafe as well.

filmaj · 2023-10-04T20:54:21Z

packages/web-api/src/WebClient.ts

   * @param options - arguments for the Web API method
   * @param headers - a mutable object representing the HTTP headers for the outgoing request
   */
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  private serializeApiCallOptions(options: WebAPICallOptions, headers?: any): string | Readable {
+  private serializeApiCallOptions(options: Record<string, unknown>, headers?: Record<string, string>): string |


Another small change to not have to use any.

filmaj · 2023-10-04T20:55:27Z

packages/web-api/src/methods.ts

@@ -1593,25 +1641,174 @@ export interface ChatPostEphemeralArguments extends WebAPICallOptions, TokenOver
  icon_url?: string; // if specified, as_user must be false
  username?: string; // if specified, as_user must be false
 }
-export interface ChatPostMessageArguments extends WebAPICallOptions, TokenOverridable {
+


This is the area of the code where I prototype modeling the chat.postMessage arguments differently, using unions and intersections instead of optional properties.

filmaj · 2023-10-04T20:56:04Z

packages/web-api/src/methods.ts

+  // Controls whether a message is a thread reply or not
+  ChatPostMessageArgumentsType;
+
+const textOnly: ChatPostMessageArguments = {


These are just tiny test cases, to show off how this woud look like.

filmaj · 2023-10-04T20:59:33Z

packages/web-api/src/methods.ts

+
+type ChatPostMessageArgumentsType = ChatPostMessageArgumentsThreadReply | ChatPostMessageArgumentsConversationMessage;
+
+type ChatPostMessageArguments =


This intersection type shows off the core of this approach: assemble the entire shape of a payload using intersections (&). Each constituent type within is a union (|). This allows us to be able to finely control when certain properties should be available, or not, or optional, in different circumstances.

filmaj · 2023-10-04T21:01:09Z

packages/web-api/src/methods.ts

+  reply_broadcast?: never;
+}
+
+type ChatPostMessageArgumentsType = ChatPostMessageArgumentsThreadReply | ChatPostMessageArgumentsConversationMessage;


This union (|) type models a message that is either a conversation message, or a reply-in-thread. Either a message does not specify neither of thread_ts and reply_broadcast (to model a conversation message, in-channel or DM), or a message has a thread_ts and an optional reply_broadcast (to model a reply-in-thread).

seratch

Great PR! Overall, the changes here look great to me

seratch · 2023-10-04T23:05:57Z

packages/web-api/src/WebClient.ts

@@ -64,10 +64,6 @@ export enum WebClientEvent {
  RATE_LIMITED = 'rate_limited',
 }

-export interface WebAPICallOptions {


This is great! but to proceed with this, we may need to review all the argument types to verify if they have latest full list. Have you already done?

seratch · 2023-10-04T23:07:10Z

packages/web-api/src/WebClient.ts

@@ -82,7 +78,6 @@ export interface WebAPICallResult {
    // `chat.postMessage` returns an array of error messages (e.g., "messages": ["[ERROR] invalid_keys"])
    messages?: string[];
  };
-  [key: string]: unknown;


If the currently generated response types miss some of response properties, this change may affect TypeScript users. That being said, we can improve it by providing complete typing as much as possible.

seratch · 2023-10-04T23:07:58Z

packages/web-api/src/WebClient.ts

   * @param method - the Web API method to call {@link https://api.slack.com/methods}
   * @param options - options
   */
-  public async apiCall(method: string, options: WebAPICallOptions = {}): Promise<WebAPICallResult> {


yeah, i don't see any issues with this change

seratch · 2023-10-04T23:08:23Z

packages/web-api/src/WebClient.ts

   * **#4**: Unless `request_file_info` set to false, call {@link https://api.slack.com/methods/files.info files.info} for
   * each file uploaded and returns that data. Requires that your app have `files:read` scope.
   * @param options
   */
-  public async filesUploadV2(options: FilesUploadV2Arguments): Promise<WebAPICallResult> {
+  public async filesUploadV2(options: FilesUploadV2Arguments): Promise<
+  WebAPICallResult & { files: FilesCompleteUploadExternalResponse[] | WebAPICallResult[] }


seratch · 2023-10-04T23:11:48Z

packages/web-api/src/methods.ts

+  /**
+   * @description The fallback text used in notifications.
+   */
+  text?: string;


Is this intentionally optional? In my undertstanding, having text is still recommended even when having blocks.

This is true. Technically, they are optional, but developers are strongly encouraged to provide one since this is an accessibility issue.

Do you think we should make it required in this SDK?

seratch · 2023-10-04T23:12:41Z

packages/web-api/src/methods.ts

+/**
+ * @description `chat.postMessage` arguments for specifying the message icon using a URL.
+ */
+interface ChatPostMessageArgumentsIconURL extends ChatPostMessageArgumentsIdentityBase {


seratch · 2023-10-04T23:14:04Z

packages/web-api/src/methods.ts

+  reply_broadcast: true,
+  icon_url: '1234',
+};
+console.log(textOnly, attachments, bs);


TODO: remove this (perhps the tests in this file too)

Yes exactly, this is just a draft to have a discussion.

filmaj · 2023-10-05T13:11:09Z

Thank you for reviewing @seratch 🙇

I think what I will do is extract the changes that address just #1323 into a separate PR, and start testing each API endpoint to make sure things work, and the types are functional for developers (i.e. no missing properties in request or response payloads, no static type errors prevent issuing API calls, and so on).

Separately / in parallel / bit by bit over time, I plan on moving API and event payloads as types into @slack/types. During that process, I will plan on redoing the payloads in a similar style as I did for chat.postMessage arguments in this PR.

filmaj · 2023-10-05T18:06:53Z

Closing this PR in favour of #1673

Filip Maj added 5 commits October 4, 2023 16:38

@slack/web-api: prep for next major release. bump min node to v18. cl…

98a706f

…eaning up the readme. remove no longer used codecov and deno build npm run scripts. update dependencies as much as possible. fix linter errors. remove a test that is no longer applicable with new axios.

Remove WebAPICallOptions, replace where it is used with `Record<strin…

2eb31e3

…g, unknown>`. Remove unneeded disabling of no-trailing-space rule. Replace use of `headers: any` in private `serializeApiCallOptions` method with `Record<string, string>`.

@slack/web-api: Remove inclusion of arbitrary properties from WebAPIC…

73e19bc

…allResult. Tweak fileUploadV2 return type as a result.

Remove optionality on abstract generic Method interface options arg, …

7edf192

…lines up behaviour with WebClient.

prototype using unions and intersections to model chat.postMessage ar…

95459e1

…guments.

filmaj requested review from seratch and a team October 4, 2023 20:44

filmaj changed the title ~~@slack/web-api~~ @slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype Oct 4, 2023

filmaj self-assigned this Oct 4, 2023

filmaj added semver:major discussion M-T: An issue where more input is needed to reach a decision area:typescript issues that specifically impact using the package from typescript projects pkg:web-api applies to `@slack/web-api` draft labels Oct 4, 2023

filmaj commented Oct 4, 2023

View reviewed changes

seratch reviewed Oct 4, 2023

View reviewed changes

filmaj mentioned this pull request Oct 5, 2023

Web API Type Safety #1673

Merged

62 tasks

filmaj closed this Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype #1670

@slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype #1670

filmaj commented Oct 4, 2023 •

edited

Loading

filmaj Oct 4, 2023 •

edited

Loading

seratch Oct 4, 2023

filmaj Oct 5, 2023

filmaj Oct 4, 2023

seratch Oct 4, 2023

filmaj Oct 4, 2023

seratch Oct 4, 2023

filmaj Oct 4, 2023

seratch Oct 4, 2023

filmaj Oct 4, 2023

filmaj Oct 4, 2023

filmaj Oct 4, 2023

filmaj Oct 4, 2023

filmaj Oct 4, 2023

seratch left a comment

seratch Oct 4, 2023

seratch Oct 4, 2023

seratch Oct 4, 2023

seratch Oct 4, 2023

seratch Oct 4, 2023

filmaj Oct 5, 2023

seratch Oct 4, 2023

seratch Oct 4, 2023

filmaj Oct 5, 2023

filmaj commented Oct 5, 2023

filmaj commented Oct 5, 2023


		type ChatPostMessageArgumentsType = ChatPostMessageArgumentsThreadReply \| ChatPostMessageArgumentsConversationMessage;

		type ChatPostMessageArguments =

@slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype #1670

@slack/web-api making request and response shapes type-safe, and chat.postMessage arguments prototype #1670

Conversation

filmaj commented Oct 4, 2023 • edited Loading

Purely Intended For Discussion

filmaj Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj commented Oct 5, 2023

filmaj commented Oct 5, 2023

filmaj commented Oct 4, 2023 •

edited

Loading

filmaj Oct 4, 2023 •

edited

Loading