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.

Sign up for GitHub

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

Jump to bottom

Fix #1148 - Adjust the app.message listener interface in TypeScript to compile the examples in documents #1185

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Fix #1148 - Adjust the app.message listener interface in TypeScript to compile the examples in documents #1185

merged 3 commits into from
Nov 2, 2021

Conversation

M1kep
Copy link
Contributor

@M1kep M1kep commented Oct 29, 2021

Summary

The goal of this PR is to provide Typescript users the ability to fully utilize the App.message method, while still providing common use case overloads with documentation.

Fixes #1148

Requirements (place an x in each [ ])

@seratch seratch added this to the 3.9.0 milestone Oct 29, 2021
@seratch seratch self-assigned this Oct 29, 2021
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented TypeScript-specific labels Oct 29, 2021
@codecov
Copy link

codecov bot commented Oct 29, 2021 •
edited
Loading

Codecov Report

Merging #1185 (a6e8bdf) into main (1b450f6) will decrease coverage by 0.09%.
The diff coverage is n/a.

❗ Current head a6e8bdf differs from pull request most recent head ed9de56. Consider uploading reports for the commit ed9de56 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
- Coverage   71.42%   71.33%   -0.10%     
==========================================
  Files          17       17              
  Lines        1414     1399      -15     
  Branches      424      415       -9     
==========================================
- Hits         1010      998      -12     
+ Misses        332      331       -1     
+ Partials       72       70       -2
Impacted Files Coverage Δ
src/App.ts 83.63% <ø> (-1.42%) ⬇️
src/receivers/SocketModeReceiver.ts 89.28% <0.00%> (+0.64%) ⬆️
src/receivers/HTTPReceiver.ts 38.04% <0.00%> (+0.95%) ⬆️

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 1b450f6...ed9de56. Read the comment docs.

seratch
seratch requested changes Oct 29, 2021
View reviewed changes
Copy link
Member

@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.

HI @M1kep, thanks for taking the time to make this pull request! Would you mind writing some test code that just verifies the mentioned patterns compile in TypeScript?

@seratch seratch changed the title Update app message overloads Fix #1148 - Adjust the app.message listener interface in TypeScript to compile the examples in documents Oct 29, 2021
filmaj
filmaj requested changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Agreed with @seratch, looks good, but some tests covering the new overload, for example, would be much appreciated.

src/App.ts Show resolved Hide resolved
stevengill
stevengill reviewed Oct 30, 2021
View reviewed changes
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for adding all of the helpful comments. If you need help with the tests, let us know!

src/App.ts Show resolved Hide resolved
@M1kep
Copy link
Contributor Author

M1kep commented Oct 30, 2021 •
edited
Loading

@stevengill Re Tests: If you're able to provide a basic example, or point me in the right direction of similar tests that'd be awesome! Otherwise, I was planning on spending some time to review Mocha and Sinon libraries

@stevengill
Copy link
Member

@M1kep I wonder if the type tests we have would be useful here? https://github.com/slackapi/bolt-js/blob/main/types-tests/message.test-d.ts

@M1kep
Add type alias for Middleware<SlackEventMiddlewareArgs<'message'>> in…
74ec6bd 
… `App`
@M1kep
Add additional overloads and comment based docs for App.message
88e0cf1 
To open this method up to TypeScript users a required either removing the two existing overloads, or creating new overloads. Hopefully by providing additional overloads as well as inline documentation, this can provide assistance as well as control

Fixes slackapi#1148
@M1kep
Add App#message Tests
ed9de56
@M1kep M1kep force-pushed the update-app-message-overloads branch from d284b2b to ed9de56 Compare November 1, 2021 02:12
seratch
seratch approved these changes Nov 1, 2021
View reviewed changes
Copy link
Member

@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.

LGTM, thanks for adding the comprehensive test patterns here 👍 Before merging this, let me wait for other maintainers' reviews for a few more business days.

@seratch seratch modified the milestones: 3.9.0, 3.8.0 Nov 1, 2021
filmaj
filmaj approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Very well done! Thanks so much for adding such thorough and well organized tests - it makes our lives much easier!

src/App.spec.ts
};

const assertMiddlewaresCalledOrder = () => {
sinon.assert.callOrder(...fakeMiddlewares);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, things I learned today: sinon.assert.callOrder! Super handy!

@seratch seratch merged commit cbab8bd into slackapi:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

@filmaj filmaj filmaj approved these changes

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

@misscoded misscoded Awaiting requested review from misscoded

@srajiang srajiang Awaiting requested review from srajiang

@stevengill stevengill Awaiting requested review from stevengill

Assignees

@seratch seratch

Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented TypeScript-specific
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

using directMention() documentation doesn't seem to be correct in docs
4 participants
@M1kep @stevengill @seratch @filmaj