slackapi / node-slack-sdk

Slack Developer Kit for Node.js
https://slack.dev/node-slack-sdk
MIT License
3.26k stars 656 forks source link

feat: allow using WebClient APIs without argument #1809

Closed davidlj95 closed 3 weeks ago

davidlj95 commented 3 weeks ago

Summary

Suggest an implementation for #1769 by using void & union types

Introduced OptionalArgument for explicitness

Fixes #1769

To pay special attention:

Requirements (place an x in each [ ])

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.62%. Comparing base (bb21de9) to head (f352c99). Report is 3 commits behind head on main.

:exclamation: Current head f352c99 differs from pull request most recent head f98f005

Please upload reports for the commit f98f005 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1809 +/- ## ======================================= Coverage 81.62% 81.62% ======================================= Files 35 35 Lines 7684 7684 Branches 316 316 ======================================= Hits 6272 6272 Misses 1400 1400 Partials 12 12 ``` | [Flag](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | Coverage Δ | | |---|---|---| | [cli-hooks](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `95.07% <ø> (ø)` | | | [cli-test](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `53.80% <ø> (ø)` | | | [oauth](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `76.51% <ø> (ø)` | | | [socket-mode](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `59.41% <ø> (ø)` | | | [web-api](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `96.48% <ø> (ø)` | | | [webhook](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/1809/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `95.20% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi#carryforward-flags-in-the-pull-request-comment) to find out more.
davidlj95 commented 3 weeks ago

Hey this looks good to me and thank you for the PR!

One note about #1769 is that we'll want to add this to all methods that accept all optional arguments. Here's a brief search I did just based on the tests where the changes in this PR would apply:

Thanks! 😊 Oh nice then 💪 I just did a few as kind of a demo of what I'd look like. If looks good, will apply it around. Thanks for searching the places where changes are needed

btw I'm thinking about adding some utility like

type OptionalArgument<T> = T | void

So the intention is more explicit and can easily be refactored around in case we need to detect in which places args are optional

Would you be able to add the same changes to all of the above methods that support purely optional arguments?

Sure!