Add API response types to web-api methods #1188

seratch · 2021-03-11T14:17:48Z

Summary

This pull request adds types to @slack/web-api API method call results. It does not bring any breaking changes. The generated types are all subtypes of WebAPICallResult interface.

Although the pull request #1078 applies similar changes, the changes were done by hand and the coverage is not complete. The approach in this PR utilizes the official Java SDK's integration test results for automatic code generation. Thus, we don't need to manually update the interface. Also, 100% of the API methods are covered.

We can assume the already generated fields are accurate as they surely exist in the actual API responses. If the Java SDK's tests do not cover some patterns, the fields that may be included only with a particular condition can be missing.

You can update the type definitions by running the following command at any time. The command checks out the Java SDK git repository and run quicktype code generator to update the TypeScript source code under packages/web-api/src/response/ package.

./scripts/generate-web-api-types.sh

We can add the execution to the CI builds but manually running it would be enough so far.

Lastly, here are pros/cons of this pull request's approach. I'm sure it's reasonable enough to go with this way though.

Pros

100% automatic code generation - zero manual updates for maintaining existing types
zero breaking changes to the existing apps
developers still can call missing fields, thanks to [key: string]: unknown; in WebAPICallResult
benefits for both TS and JS (+VS Code) users

Cons

dependency to the slack-java-sdk project
hundreds of new files
depends on quicktype

If we have to stop using quicktype in the future, we can just give up automatic generation. We are still able to manually maintain the types afterward.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

seratch

comments for reviewers

seratch · 2021-03-11T14:19:39Z

packages/web-api/src/WebClient.spec.js

-
-  describe('constructor()', function () {
-    it('should build a default client given a token', function () {
+require("mocha");


I just applied prettier to all the files in web-api project

oh, lint fails. i will update the whole changes.

seratch · 2021-03-11T14:20:34Z

packages/web-api/src/methods.ts

+  WebClientEvent,
+} from "./WebClient";
+import { EventEmitter } from "eventemitter3";
+import { AdminAppsApproveResponse } from "./response/AdminAppsApproveResponse";


These files under ./response are auto generated.

seratch · 2021-03-11T14:21:09Z

packages/web-api/src/methods.ts


  public readonly admin = {
    apps: {
-      approve: bindApiCall<AdminAppsApproveArguments, WebAPICallResult>(this, 'admin.apps.approve'),
+      approve: bindApiCall<AdminAppsApproveArguments, AdminAppsApproveResponse>(


AdminAppsApproveResponse is a subtype of WebAPICallResult

seratch · 2021-03-11T14:22:14Z

packages/web-api/src/response/ChatPostMessageResponse.ts

@@ -0,0 +1,363 @@
+import { WebAPICallResult } from "../WebClient";
+export type ChatPostMessageResponse = WebAPICallResult & {


Probably, most of you are already familiar with this type 😄

seratch · 2021-03-11T15:22:13Z

codecov does not return the results but the builds passed!

seratch · 2021-03-11T22:00:38Z

packages/web-api/src/response/AdminAnalyticsGetFileResponse.ts

@@ -0,0 +1,8 @@
+/* tslint:disable */


Some parts of auto-generated code are not compatible with the current tslint settings. I tried to settle all of them this way but a few patterns are inevitable. You can see the result by modifying the code generator.

seratch · 2021-03-11T22:04:34Z

Let me add a few more comments for some questions about this:

Q: What's the dependency to the Java SDK project?
To be clear, this code generation depends on only JSON data produced by the integration tests in the Java SDK. There is no dependency to Java runtime and Java SDK code. These JSON files are automatically generated from the actual JSON response from our server-side. The test utilities in the project save and normalize the data in this directory. If we do the same in the Node project as part of integration tests, we can do this only in the node-slack-sdk project. But it’s obviously duplicated work.

Q: Why are all fields optional?
Being required or optional is not easily deterministic for machine at this point. The code generator uses normalized JSON data as source of truth and there is no metadata about optional vs non-optional. Also, it can often be complex. Some fields can be basically optional while it may be non-optional under a particular condition. It may be possible to try to implement custom rules but we are unable to use existing tools like quicktype for it. We have to invent our own tool or manually maintain types. In my opinion, these ways are not sustainable.

mwbrooks

I like this approach. Well done @seratch!

While auto-generated types may not be as rich as handwritten, it's better than nothing and easier to maintain.

The dependency on the Java SDK is not ideal, but okay in my mind. If it's an issue, we could create separate project to generate the types and send a PR into this project.

@seratch is it possible to switch the Ruby quicktype for Node.js dev dependency?

npm install --save-dev quicktype

seratch · 2021-03-12T01:50:51Z

@mwbrooks Thanks for the suggestion - I've resolved it 👍

TonalidadeHidrica · 2021-04-14T04:01:53Z

I'm very excited to use the fully-typed version of API client, but it's not released yet. The issues in 6.2 milestone has all been closed or merge, so isn't it already possible to release the new version?

seratch · 2021-04-15T01:36:57Z

@TonalidadeHidrica Thanks for your interest! Yes, the last task in the list was completed yesterday. We are going to release a new version soon 🚀

seratch added enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Mar 11, 2021

seratch requested review from mwbrooks, stevengill and misscoded March 11, 2021 14:17

seratch self-assigned this Mar 11, 2021

seratch commented Mar 11, 2021

View reviewed changes

seratch force-pushed the web-api-response-types branch from 32eabbc to 94eb35a Compare March 11, 2021 15:15

seratch commented Mar 11, 2021

View reviewed changes

mwbrooks approved these changes Mar 12, 2021

View reviewed changes

Add API response types to WebClient

40466cc

seratch force-pushed the web-api-response-types branch from e7e7fce to 40466cc Compare March 12, 2021 01:53

seratch mentioned this pull request Mar 15, 2021

Developer experience building Slack apps with Bolt using TypeScript slackapi/bolt-js#826

Closed

10 tasks

seratch merged commit 156027d into slackapi:main Mar 18, 2021

seratch deleted the web-api-response-types branch March 18, 2021 05:13

seratch mentioned this pull request Mar 18, 2021

Add response types to Web API methods #1078

Closed

2 tasks

seratch added this to the web-api@6.2 milestone Mar 24, 2021

seratch mentioned this pull request Apr 30, 2021

Strong type api method responses #509

Closed

9 tasks

seratch mentioned this pull request May 13, 2021

Fix #1235 ViewsOpenResponse type is incorrect #1236

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API response types to web-api methods #1188

Add API response types to web-api methods #1188

seratch commented Mar 11, 2021

seratch left a comment

seratch Mar 11, 2021

seratch Mar 11, 2021

seratch Mar 11, 2021

seratch Mar 11, 2021

seratch Mar 11, 2021

seratch Mar 11, 2021

seratch commented Mar 11, 2021

seratch Mar 11, 2021

seratch commented Mar 11, 2021

mwbrooks left a comment

seratch commented Mar 12, 2021

TonalidadeHidrica commented Apr 14, 2021

seratch commented Apr 15, 2021

		@@ -0,0 +1,363 @@
		import { WebAPICallResult } from "../WebClient";
		export type ChatPostMessageResponse = WebAPICallResult & {

Add API response types to web-api methods #1188

Add API response types to web-api methods #1188

Conversation

seratch commented Mar 11, 2021

Summary

Pros

Cons

Requirements (place an x in each [ ])

seratch left a comment

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch commented Mar 11, 2021

seratch Mar 11, 2021

Choose a reason for hiding this comment

seratch commented Mar 11, 2021

mwbrooks left a comment

Choose a reason for hiding this comment

seratch commented Mar 12, 2021

TonalidadeHidrica commented Apr 14, 2021

seratch commented Apr 15, 2021

Requirements (place an `x` in each `[ ]`)