feat: allow using WebClient APIs without argument #1809

davidlj95 · 2024-06-10T12:36:23Z

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 `[ ]`)

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.

codecov · 2024-06-10T15:33:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@           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	Coverage Δ
cli-hooks	`95.07% <ø> (ø)`
cli-test	`53.80% <ø> (ø)`
oauth	`76.51% <ø> (ø)`
socket-mode	`59.41% <ø> (ø)`
web-api	`96.48% <ø> (ø)`
webhook	`95.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

filmaj

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:

➜ ack "all optional" test/types
test/types/methods/admin.emoji.test-d.ts
42:expectAssignable<Parameters<typeof web.admin.emoji.list>>([{}]); // all optional args

test/types/methods/admin.conversations.test-d.ts
160:}]); // all optional args
293:}]); // all optional args

test/types/methods/usergroups.test-d.ts
37:expectAssignable<Parameters<typeof web.usergroups.list>>([{}]); // all optional properties

test/types/methods/admin.barriers.test-d.ts
56:expectAssignable<Parameters<typeof web.admin.barriers.list>>([{}]); // all optional args

test/types/methods/dnd.test-d.ts
10:expectAssignable<Parameters<typeof web.dnd.endDnd>>([{}]); // all optional args
16:expectAssignable<Parameters<typeof web.dnd.endSnooze>>([{}]); // all optional args
22:expectAssignable<Parameters<typeof web.dnd.info>>([{}]); // all optional args

test/types/methods/openid.test-d.ts
28:expectAssignable<Parameters<typeof web.openid.connect.userInfo>>([{}]); // all optional args

test/types/methods/api.test-d.ts
10:expectAssignable<Parameters<typeof web.api.test>>([{}]); // all optional args

test/types/methods/rtm.test-d.ts
10:expectAssignable<Parameters<typeof web.rtm.connect>>([{}]); // all optional arguments
16:expectAssignable<Parameters<typeof web.rtm.start>>([{}]); // all optional arguments

test/types/methods/admin.teams.test-d.ts
35:expectAssignable<Parameters<typeof web.admin.teams.list>>([{}]); // all optional

test/types/methods/team.test-d.ts
10:expectAssignable<Parameters<typeof web.team.accessLogs>>([{}]); // all optional arguments
16:expectAssignable<Parameters<typeof web.team.billableInfo>>([{}]); // all optional arguments
22:expectAssignable<Parameters<typeof web.team.billing.info>>([{}]); // all optional arguments
28:expectAssignable<Parameters<typeof web.team.integrationLogs>>([{}]); // all optional arguments
34:expectAssignable<Parameters<typeof web.team.profile.get>>([{}]); // all optional arguments
40:expectAssignable<Parameters<typeof web.team.preferences.list>>([{}]); // all optional arguments

test/types/methods/auth.test-d.ts
10:expectAssignable<Parameters<typeof web.auth.revoke>>([{}]); // all optional args
16:expectAssignable<Parameters<typeof web.auth.teams.list>>([{}]); // all optional args
22:expectAssignable<Parameters<typeof web.auth.test>>([{}]); // all optional args

test/types/methods/admin.roles.test-d.ts
52:expectAssignable<Parameters<typeof web.admin.roles.listAssignments>>([{}]); // all optional

test/types/methods/reactions.test-d.ts
58:expectAssignable<Parameters<typeof web.reactions.list>>([{}]); // all optional args

test/types/methods/conversations.test-d.ts
174:expectAssignable<Parameters<typeof web.conversations.list>>([{}]); // all optional args
180:expectAssignable<Parameters<typeof web.conversations.listConnectInvites>>([{}]); // all optional args

test/types/methods/bots.test-d.ts
10:expectAssignable<Parameters<typeof web.bots.info>>([{}]); // all optional args

test/types/methods/reminders.test-d.ts
79:expectAssignable<Parameters<typeof web.reminders.list>>([{}]); // all optional args

test/types/methods/chat.test-d.ts
375:expectAssignable<Parameters<typeof web.chat.scheduledMessages.list>>([{}]); // all optional args

test/types/methods/admin.apps.test-d.ts
10:expectAssignable<Parameters<typeof web.admin.apps.activities.list>>([{}]); // all optional args

test/types/methods/emoji.test-d.ts
10:expectAssignable<Parameters<typeof web.emoji.list>>([{}]); // all optional args

test/types/methods/apps.test-d.ts
10:expectAssignable<Parameters<typeof web.apps.connections.open>>([{}]); // all optional args

test/types/methods/admin.users.test-d.ts
79:expectAssignable<Parameters<typeof web.admin.users.list>>([{}]); // all optional args is ok
157:expectAssignable<Parameters<typeof web.admin.users.session.list>>([{}]); // all optional args is OK
264:expectAssignable<Parameters<typeof web.admin.users.unsupportedVersions.export>>([{}]); // all optional args OK

test/types/methods/admin.workflows.test-d.ts
73:expectAssignable<Parameters<typeof web.admin.workflows.search>>([{}]); // all optional args OK

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

davidlj95 · 2024-06-10T15:44:21Z

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!

davidlj95 · 2024-06-10T17:33:18Z

packages/web-api/src/types/request/apps.ts


 // https://api.slack.com/methods/apps.connections.open
-export interface AppsConnectionsOpenArguments { }
+export type AppsConnectionsOpenArguments = OptionalArgument<object>;


⚠️ BREAKING CHANGE (technically, not pragmatically)

OpenIDConnectUserInfoArguments & AppsConnectionsOpenArgument are declared as empty interfaces.

An empty interface allows for any non nullable value, not an object with no properties. It is well explained in typescript-eslint rule no-empty-interface. In order to specify an empty object, object can be used. Using that then

This would be a breaking change if someone is using a non-nullable and non-object value as argument to one of those methods. Like apps.connections.open(42). But that shouldn't have worked in the first place

Can use {} to keep same type as before and ignore ESLint rule complaining about it if we want to be sure no breaking change happens (could be done in next major as cleanup?)

Huh, TIL! I think as long as the previous "expected" usage of these methods (i.e. needing to provide an empty object {} as a parameter for these methods that accept optional arguments) are a) enshrined in tests and b) pass, then that's all that matters to me.

Same here tbh! Didn't know about that either, it was ESLint rule complaining that lead me through the rabbit hole.

Noice then 👍

davidlj95 · 2024-06-10T17:36:43Z

packages/web-api/src/types/request/team.ts

 // https://api.slack.com/methods/team.info
-export interface TeamInfoArguments extends TokenOverridable {
+export type TeamInfoArguments = OptionalArgument<TokenOverridable & {
  /**


☝️ This one has no type tests, but it's optional too

Can we add missing type tests while we're here?

Done in f98f005

filmaj

Thanks for the work and explanation around extending from empty interfaces! I learned something today 😄

If there is a missing test, then if you could please add it, that would be awesome! Otherwise everything looks good to me.

filmaj · 2024-06-10T18:02:10Z

packages/web-api/src/types/request/apps.ts


 // https://api.slack.com/methods/apps.connections.open
-export interface AppsConnectionsOpenArguments { }
+export type AppsConnectionsOpenArguments = OptionalArgument<object>;


Huh, TIL! I think as long as the previous "expected" usage of these methods (i.e. needing to provide an empty object {} as a parameter for these methods that accept optional arguments) are a) enshrined in tests and b) pass, then that's all that matters to me.

filmaj · 2024-06-10T18:03:14Z

packages/web-api/src/types/request/team.ts

 // https://api.slack.com/methods/team.info
-export interface TeamInfoArguments extends TokenOverridable {
+export type TeamInfoArguments = OptionalArgument<TokenOverridable & {
  /**


Can we add missing type tests while we're here?

davidlj95 · 2024-06-10T18:47:03Z

packages/web-api/test/types/methods/team.test-d.ts

+// team.info
+// -- sad path
+// -- happy path
+expectAssignable<Parameters<typeof web.team.info>>([{}]); // all optional arguments
+expectAssignable<Parameters<typeof web.team.info>>([]); // no arg is fine


🆕 team.info API type tests

feat: allow using (auth|api).test without args

d51c601

salesforce-cla bot added the cla:signed label Jun 10, 2024

filmaj suggested changes Jun 10, 2024

View reviewed changes

davidlj95 added 2 commits June 10, 2024 18:22

refactor: introduce OptionalArgument utility type

0fff391

feat: allow no args usage in admin APIs

fb79423

davidlj95 changed the title ~~feat: allow using (auth|api).test without args~~ feat: allow using WebClient APIs without argument Jun 10, 2024

davidlj95 added 3 commits June 10, 2024 19:08

feat: allow no arg usage in rest of APIs

1122ce1

style: undo import sorting to reduce diff

f352c99

chore: remove unneeded eslint ignore comment

79d6b16

davidlj95 commented Jun 10, 2024

View reviewed changes

davidlj95 requested a review from filmaj June 10, 2024 17:37

filmaj approved these changes Jun 10, 2024

View reviewed changes

filmaj added semver:patch enhancement M-T: A feature request for new functionality area:typescript issues that specifically impact using the package from typescript projects pkg:web-api applies to `@slack/web-api` auto-triage-skip labels Jun 10, 2024

filmaj added this to the [email protected] milestone Jun 10, 2024

test: add team.info web client type test

f98f005

davidlj95 commented Jun 10, 2024

View reviewed changes

filmaj merged commit 9f2935f into slackapi:main Jun 10, 2024
19 checks passed

davidlj95 deleted the feat-allow-test-no-args branch June 10, 2024 18:54

filmaj modified the milestones: [email protected], [email protected] Jun 11, 2024

This was referenced Jul 4, 2024

[Snyk] Upgrade @slack/web-api from 7.0.4 to 7.1.0 petersem/monocker#96

Open

[Snyk] Upgrade @slack/web-api from 7.0.4 to 7.2.0 petersem/monocker#98

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow using WebClient APIs without argument #1809

feat: allow using WebClient APIs without argument #1809

davidlj95 commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

filmaj left a comment

davidlj95 commented Jun 10, 2024

davidlj95 Jun 10, 2024

davidlj95 Jun 10, 2024 •

edited

Loading

filmaj Jun 10, 2024

davidlj95 Jun 10, 2024

davidlj95 Jun 10, 2024

filmaj Jun 10, 2024

davidlj95 Jun 10, 2024

davidlj95 Jun 10, 2024

filmaj left a comment

filmaj Jun 10, 2024

filmaj Jun 10, 2024

davidlj95 Jun 10, 2024

feat: allow using WebClient APIs without argument #1809

feat: allow using WebClient APIs without argument #1809

Conversation

davidlj95 commented Jun 10, 2024 • edited Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

filmaj left a comment

Choose a reason for hiding this comment

davidlj95 commented Jun 10, 2024

Choose a reason for hiding this comment

davidlj95 Jun 10, 2024 • 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

filmaj 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

davidlj95 commented Jun 10, 2024 •

edited

Loading

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

codecov bot commented Jun 10, 2024 •

edited

Loading

davidlj95 Jun 10, 2024 •

edited

Loading