slackapi / node-slack-sdk

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

types 3.0: breaking changes for block kit types #1905

Open filmaj opened 2 weeks ago

filmaj commented 2 weeks ago

Taking inspiration from @seratch's library, https://github.com/seratch/slack-web-api-client/tree/main/src/block-kit, should study the following to see what kinds of improvements on the Block Kit side we could leverage in @slack/types.

Also, we should study bolt-js' use of composable event payloads and recursive Block Kit types, to see what improvements could land here. See the issues tagged in https://github.com/slackapi/node-slack-sdk/issues/1904

Idea: Add generics to layout blocks

What if we extended Blocks with generics, so that a containing layout block could constrain or fully specify the block elements contained within. An example on how this would look like for a dev:

const myImage: ImageElement = {
  type: 'image',
  alt_text: 'kitteh',
  image_url: 'https://kittens-r-us.com/kitteh.jpg',
}
const myContextBlock: ContextBlock<ImageElement> = {
  type: 'context',
  elements: [myImage], // compiles fine
}
const nopeBlock: ContextBlock<ImageElement> = {
  type: 'context',
  elements: [{ type: 'text', text: 'hello' }], // nope, TS complains that { type: 'text' } not assignable to ImageElement
}

The benefit here is more control and composability. Extending this further, we could employ a similar approach to other Slack domain objects and event payloads, so that they could all be composed in a similar way. For example, a block_actions event payload from a button click could be modeled by passing the ButtonElement into the BlockActionsEvent via generic (BlockActionsEvent<ButtonElement>), or the same idea for composing a View, and then passing that View into a ViewSubmissionEvent.. and so on.

Other Changes

seratch commented 2 weeks ago

Nice!

ContextBlock

This may not work out because a context object can a few different elements at a time. Thus, just making the elements property type more specific would be a reasonable improvement: https://github.com/seratch/slack-web-api-client/blob/1.0.3/src/block-kit/blocks.ts#L95

Aside from that, we may want to give more attention to both block types and block element types, that are acceptable under a condition. More specificallty, message data accepts these blocks while home tab and modal data are different: https://github.com/seratch/slack-web-api-client/blob/1.0.3/src/block-kit/blocks.ts#L39-L71

@slack/web-api currently accepts any blocks for chat.postMessage API in TS, but we can make it more specific this way.

Even if we introduce the type changes in the next major version, most currently working code should not have any runtime issues. Thus, I agree that this change is worth considering for better developer experience in the long run. The only pain points developers would have for the migration would be udpating explicitly set types on the user side (e.g., having KnownBlock[]; within a developers' code).

filmaj commented 1 week ago

Thanks for commenting @seratch ! I was hoping to generate some discussion to test some of my assumptions and ideas and see if they could work or not, if they are useful or not.

This may not work out because a context object can a few different elements at a time.

I should be more specific / detailed in my proposal 😅 . What prompted this idea of using generics for layout blocks was trying to better model the the "Legacy Fields" of Secondary Attachments. In particular, the author_icon field accepts a Context Block with an Image Element. As you pointed out @seratch, by default the Context Block accepts arrays of both image and text elements - but not author_icon! It is a bit more constrained (images only).

Let me expand my pseudocode example; what if by default the ContextBlock generic I described in my original post assigned the generic to be a union of Image and Text elements (meeting the default constraints for Context), but allowed overwriting it to constrain it even more in certain situations (like author_icon in secondary attachments)? E.g.:

type ContextElements = ImageElement | TextElement;

// by default, either text or image elements can be assigned to ContextBlock
interface ContextBlock<Elements extends ContextElements = ContextElements> {
  elements: Elements[];
}
// .. but we can override it in more constrained situations, like `author_icon` in Secondary Attachments:
interface Attachment {
  author_icon: ContextBlock<ImageElement>;
}

This way the 'standard' use of ContextBlock would be satisfied, but more constrained versions could also be defined, too.

Aside from that, we may want to give more attention to both block types and block element types, that are acceptable under a condition.

I like this; it seems to mirror the "Available in surfaces" column of the layout block breakdown on our documentation.

@slack/web-api currently accepts any blocks for chat.postMessage API in TS, but we can make it more specific this way.

Love this!

The only pain points developers would have for the migration would be updating explicitly set types on the user side

Indeed, but as part of any major version release, I plan on writing a migration guide for the most common breaking change scenarios, and this scenario would be covered in that document.

filmaj commented 1 week ago

I've added "define utility types for availability of blocks in different Slack surfaces" as a task on this issue, but technically this could be a new utility type exposed in v2.x of types - a minor change, not a major one. Perhaps worth addressing that earlier and releasing in types v2.x - what do you think?