Reviewing Types #1542
Reviewing Types #1542
Conversation
…composition objects), updated optional/required state on many properties, added missing properties on some interfaces.
filmaj
added
enhancement
packages/types/src/index.ts
Outdated
| initial_user?: string; | ||
| placeholder?: PlainTextElement; | ||
| placeholder: PlainTextElement; |
There was a problem hiding this comment.
I am sure that this is optional, so this change should be reverted. The document is wrong! See a Block Kit Builder example
There was a problem hiding this comment.
You are correct! I was very surprised but confirmed that for all these Block Elements, both placeholder and action_id are optional. I am working on a PR to update the docs. Thanks for pointing this out, I have updated these to set placeholder and action_id to optional.
packages/types/src/index.ts
Outdated
| initial_users?: string[]; | ||
| placeholder?: PlainTextElement; | ||
| placeholder: PlainTextElement; |
packages/types/src/index.ts
Outdated
| max_selected_items?: number; | ||
| confirm?: Confirm; | ||
| focus_on_load?: boolean; | ||
| } | ||
|
|
||
| export interface StaticSelect extends Action { | ||
| type: 'static_select'; | ||
| placeholder?: PlainTextElement; | ||
| placeholder: PlainTextElement; | ||
| action_id: string; |
There was a problem hiding this comment.
nit: when you build a block element, setting action_id is optional (auto-generated one will be used in the case). thus, strictly speaking, this can be optional
There was a problem hiding this comment.
Are you sure action_id is optional? The docs say it is required. I know block_id is optional and one will be auto-generated for you in that case.
I will test this today in real apps and report back!
There was a problem hiding this comment.
Wow I can't believe it, action_id is optional 😆
packages/types/src/index.ts
Outdated
| initial_options?: PlainTextOption[]; | ||
| options?: PlainTextOption[]; | ||
| options?: PlainTextOption[]; // TODO: options and option_groups are mutually exclusive |
There was a problem hiding this comment.
good point but i am unsure how we can represent this type of constraints in a simple way
There was a problem hiding this comment.
I will ask Neil who is the deno-slack-sdk type master 😄
|
@filmaj For better testing, checking the compatibility with actual production payload data may be helpful. I have been doing so for python/java projects, which is quite beneficial for detecting errors in real life. |
|
@seratch sounds good, I was planning on doing that yesterday but ran out of time. I will do so today. |
…ored action_id into top-level Action type.
Co-authored-by: Sarah Jiang <sarahjiang@slack-corp.com>
packages/types/src/index.ts
Outdated
| @@ -226,6 +241,7 @@ export interface ExternalSelect extends Action { | |||
|
|
|||
| export interface MultiExternalSelect extends Action { | |||
| type: 'multi_external_select'; | |||
| // event to return option data. so de-facto this is necessary | |||
There was a problem hiding this comment.
I'm not certain I understand what this comment means! Is this a note to self?
There was a problem hiding this comment.
LOL it was a much longer comment that I had before that I then decided to remove which I clearly didn't do a great job of 😆
| */ | ||
| block_id?: string; // TODO: in event payloads, this property will always be present. When defining a block, however, | ||
| // it is optional. If not defined when the block is created, Slack will auto-assign a block_id for you - thus why it | ||
| // is always present in payloads. So: how can we capture that? Differentiate between the event payload vs. the block |
There was a problem hiding this comment.
We probably have to, since I don't see a world where this particular behavior changes - and the behavior seems reasonable. Were you thinking that this Block Kit type could somehow be used to model event payload responses?
There was a problem hiding this comment.
Yes exactly! At least, I was consider if that would be possible! If you look at the block_actions payload event's fields, you'll see that there is an array of actions provided in the payload. These actions are the exact block kit elements that were interacted with - so I was considering if it would be possible to re-use the types in this repo to try to model the items of that actions array in the block_actions payload.
There was a problem hiding this comment.
As for the item in actions in a block_actions payload, we already have complete types in bolt-js. If we would like to have it, we can consider moving the ones in bolt-js into this repo. Reusing Action and so on here may cause unnecessary complexity.
There was a problem hiding this comment.
@seratch Yes I would like to eventually move as many types for things shared across repos as possible into this repo. For the block_actions payload, for example, I know right now we only need it in bolt-js. However, in deno-slack-sdk, it is also needed. So all of this work is in prep for trying to consolidate not just around our existing node projects, but across our newer deno ones too.
There was a problem hiding this comment.
Thanks for the review add and so glad you're working on this (mostly because of all the adjacent improvements to our documentation this work becomes a forcing function too...)! I don't have much to add wrt to correctness of these types but:
wrt: Testing
👍 to more manual testing needed, and wondering whether we can spin off a couple of concrete tasks around incorporating into sample apps.
wrt: release strategy
An rc release of this off a branch while we're testing sounds good to me! Any strong benefit you see to incorporating into main at this early stage?
RC release seems safe to me at this stage, yeah. I do plan on adding a ton of niceties to this repo (JSdocs, links to api.slack.com, etc.), so releasing to main at some point would be a nice little present for our devs 😊 |
|
So maybe instead of merging this PR into the |
|
I've started to release this branch under the |
Summary
This is my first pass at reviewing the types in this repo in preparation for wanting to consume them in the deno-slack-sdk.
I used the following api.slack.come pages as part of this review:
I tried to organize a majority of the types in this module into roughly three categories based on the above api.slack.com pages.
Added some missing properties, updated what is optional and what is not based on the above api.slack.com pages. I left some TODOs in, as well, based on what issues I saw that I did not feel like tackling immediately.
TODO
I do want to split this file up into smaller, more focussed/thematic files - but I will do that in a follow-up PR. I want to also put in some time to add nice JSDoc comments to all interfaces and types, I think that would improve the developer experience as well. Initially, however, I just want the properties to be reflected accurately so that I can test this repo in bolt-js, web-api and in deno-slack-sdk apps, and see how they work together.
Testing
Speaking of testing: I tested this with both the
@slack/web-apiand@slack/boltrepos. Both repos' unit tests pass without issue. That said, I am wondering if this is a false negative... likely needs more testing in a real app.Releasing
Not sure if this should be released / merged into
main, or if this should maybe be released as a RC/alpha/beta style release on npm in the short term, until more complete manual testing can be complete. Thoughts?