bolt-js icon indicating copy to clipboard operation
bolt-js copied to clipboard

Blocks in client.conversations.history are different to blocks that use in message posting

Open funtaps opened this issue 2 years ago • 8 comments

Description

If I want to copy message from one channel to another, or if I want to edit something in message, when I use TS and bolt js, I can't do it.

For example:

    const result = await client.conversations.history({
      channel: FROM_CHANNEL,
      latest: TARGET_TS,
      inclusive: true,
      limit: 1,
    });
    if (result.messages !== undefined) {
      const message = result.messages[0];
      if (message) {
      await client.chat.postMessage({
        channel: TO_CHANNEL,
        text: message.text,
        blocks: message.blocks,
      });
      await client.chat.update({
        channel: FROM_CHANNEL,
        ts: TARGET_TS,
        blocks: [{type: 'divider'}, ...message.blocks],
      });
    }

In both cases, I've got error with blocks field. ConversationsHistoryResponse Block have many differences to normal Block type. For example field type declared to be string|undefined in first, and string in second. Are those types correspond to reality? If so, maybe we can create some converter, that will check blocks from conversations.history and fix them, so they will be OK to use as KnownBlocks?

What type of issue is this? (place an x in one of the [ ])

  • [ ] bug
  • [x] enhancement (feature request)
  • [ ] question
  • [ ] documentation related
  • [ ] example code related
  • [ ] testing related
  • [ ] discussion

Requirements (place an x in each of the [ ])

  • [ x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • [x ] I've read and agree to the Code of Conduct.
  • [x ] I've searched for any related issues and avoided creating a duplicate issue.

funtaps avatar Feb 10 '23 12:02 funtaps

Hi @funtaps thanks for writing in 💯 I'm sure others may have bumped in this issue

I've compared the payload of conversation.history blocks with the payload of chat.update blocks and they seem to be semantically equivalent.

Conversation.history response example show that messages may not have a blocks field, it could be possible that one of your messages does not contain a blocks field, this could lead to result.messages[0].blocks = undefined

Could you try adding a check for blocks as follows

if (message && message.blocks) {
      await client.chat.postMessage({
        channel: TO_CHANNEL,
        text: message.text,
        blocks: message.blocks,
      });
      await client.chat.update({
        channel: FROM_CHANNEL,
        ts: TARGET_TS,
        blocks: [{type: 'divider'}, ...message.blocks],
      });

Would you be able to you provide the JSON of the message containing the blocks that fail?

WilliamBergamin avatar Feb 10 '23 16:02 WilliamBergamin

I do not know, if there are some messages that will fail. I'm using TS, and I get errors not in runtime but at compile time. It is ok, if blocks are undefined, but problem is in Block type itself.

This is type of Block that is returned by converstion.history https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/response/ConversationsHistoryResponse.ts#L152

export interface Block {
  ...
  type?:                     string;
  ...
}

And this is the type of Block that is ok to be arguments in postMessage or updateMessage https://github.com/slackapi/node-slack-sdk/blob/master/packages/types/src/index.ts#L384

export interface Block {
  type: string;
  block_id?: string;
}

I do not know, is it possible for conversation.history to return messages with blocks, but to some of this blocks to have no type. But typings suggest that that is the case. I see that code, that I am referring to is in node-slack-sdk. Maybe I need to move my issue there?

funtaps avatar Feb 10 '23 19:02 funtaps

I see, thank you for clearing things up 🥇

The type field in blocks should always be required, but we may want to set this field as optional everywhere to prevent breaking changes, ideally we would want to reference the blocks interface instead of copying it. I'll label this as a good first issue

WilliamBergamin avatar Feb 10 '23 20:02 WilliamBergamin

but we may want to set this field as optional everywhere to prevent breaking changes

It should not be optional, when you use it as arguments: it is great to see error message, if I am trying to post a message with blocks without type. The only real concern is - what can I get as a result of https://api.slack.com/methods/conversations.history ? Can there really be blocks without types? If yes, then typings are correct, and maybe there should be some converter helper function. But I think to have a good one, there should be an understanding, what are the cases, where ConversationsHistoryResponse have such peculiar blocks. If we can guarantee, that if you have a response from conversations.history, that have messages with blocks, than those blocks are KnownBlock type, then we just should fix the typings.

In summary my question is are ConversationsHistoryResponse typings correct, or they are too abstract, and in fact we can be sure, of some truths, that are not described by this types https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/response/ConversationsHistoryResponse.ts#L152 ?

funtaps avatar Feb 10 '23 20:02 funtaps

@funtaps

The only real concern is - what can I get as a result of https://api.slack.com/methods/conversations.history ? Can there really be blocks without types?

No, every single block must have its type, so the type exists. No exception.

In summary my question is are ConversationsHistoryResponse typings correct, or they are too abstract, and in fact we can be sure, of some truths, that are not described by this types https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/response/ConversationsHistoryResponse.ts#L152 ?

The Web API response types are auto-generated, and as of today, all the properties are optional due to technical limitation of the underlying tools and source data (the source data is much harder to solve). For this reason, some of the properties such as top-level ok and blocks[].type and so on are typed as optional while they always exist.

Let me share a bit more context behind this. A few years, the response types did not exist. All the web api responses were just any at that time. And then, I've added the auto-generated types as a "better-than-nothing" solution. I hear you that the types could be confusing, however it'd be appreciated if you could understand where we are now.

@WilliamBergamin marked this issue as a bug, but unfortunately I don't anticipate our team can improve web api response properties in the short term (more specifically, even within a few years). So, if @funtaps does not have any follow up questions on this topic, let us close this issue.

seratch avatar Feb 21 '23 04:02 seratch

Right now there is no good way to use conversations.history messages to copy or update - that is kinda sad. If we know that api returns in fact KnownBlock[], maybe it is possible to use it in type definition of history method? Or maybe I can add some converter functions, with signature convertResultBlockToKnownBlock(HistoryAPIBlock block): KnownBlock I'll need to do this for my code, so maybe there is a benefit to put it there for everyone. It can even check the correctness with something like zod.

funtaps avatar Mar 05 '23 09:03 funtaps

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out. If you think this issue needs to be prioritized, please comment to get the thread going again! Maintainers also review issues marked as stale on a regular basis and comment or adjust status if the issue needs to be reprioritized.

github-actions[bot] avatar Apr 10 '23 00:04 github-actions[bot]

I am going to take a look at working on this issue

cbullard-dev avatar Apr 17 '23 02:04 cbullard-dev