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

Make API response types more specific utilizing the types in web-api 6.2 #915

Merged
merged 2 commits into from
Jun 3, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented May 12, 2021

Summary

This pull request updates a few parts of this library to utilize the newly introduced more specific API response types. As all the new types inherit WebAPICallResult interface, these change are backward-compatible.

Requirements (place an x in each [ ])

@seratch seratch added enhancement M-T: A feature request for new functionality semver:minor TypeScript-specific labels May 12, 2021
@seratch seratch added this to the 3.4.0 milestone May 12, 2021
@seratch seratch self-assigned this May 12, 2021
Copy link
Member Author

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments for reviewers

@@ -46,7 +46,7 @@
"@slack/oauth": "^2.0.0",
"@slack/socket-mode": "^1.0.0",
"@slack/types": "^2.0.0",
"@slack/web-api": "^6.0.0",
"@slack/web-api": "^6.2.3",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this PR require the latest version of the package.

@@ -148,6 +148,8 @@ export default class SocketModeReceiver implements Receiver {

public start(): Promise<void | WebAPICallResult> {
// start socket mode client
// TODO: We can update the returned type from WebAPICallResult to AppsConnectionsOpenResponse
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to hold off merging this PR until this TODO fix if others think we should do so.

Copy link
Member

@mwbrooks mwbrooks May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to hold off on merging the PR. What's blocking us from updating the returned type to AppsConnectionsOpenResponse?

Edit: Oh, I just finished reviewing slackapi/node-slack-sdk#1234 and noticed it updates AppsConnectionsOpenResponse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update this part after releasing @slack/[email protected] - slackapi/node-slack-sdk#1239

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #915 (1ff444e) into main (0ec4617) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #915   +/-   ##
=======================================
  Coverage   66.19%   66.19%           
=======================================
  Files          13       13           
  Lines        1207     1207           
  Branches      356      356           
=======================================
  Hits          799      799           
  Misses        338      338           
  Partials       70       70           
Impacted Files Coverage Δ
src/WorkflowStep.ts 91.01% <ø> (ø)
src/receivers/SocketModeReceiver.ts 11.53% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ec4617...1ff444e. Read the comment docs.

@@ -148,6 +148,8 @@ export default class SocketModeReceiver implements Receiver {

public start(): Promise<void | WebAPICallResult> {
// start socket mode client
// TODO: We can update the returned type from WebAPICallResult to AppsConnectionsOpenResponse
Copy link
Member

@mwbrooks mwbrooks May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to hold off on merging the PR. What's blocking us from updating the returned type to AppsConnectionsOpenResponse?

Edit: Oh, I just finished reviewing slackapi/node-slack-sdk#1234 and noticed it updates AppsConnectionsOpenResponse.

@seratch seratch force-pushed the web-api-6.2-updates branch from 1b50377 to 1ff444e Compare June 3, 2021 09:13
@seratch seratch merged commit ca52855 into slackapi:main Jun 3, 2021
@seratch seratch deleted the web-api-6.2-updates branch June 3, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:minor TypeScript-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants