Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 7 tasks
funtaps opened this issue Feb 10, 2023 · 8 comments
Open
1 of 7 tasks
Labels
auto-triage-skip Prevent this issue from being closed due to lack of activity discussion M-T: An issue where more input is needed to reach a decision enhancement M-T: A feature request for new functionality good first issue Good for newcomers TypeScript-specific

Comments

@funtaps
Copy link
Contributor

funtaps commented Feb 10, 2023

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
  • 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.

@WilliamBergamin
Copy link
Contributor

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?

@funtaps
Copy link
Contributor Author

funtaps commented Feb 10, 2023

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?

@WilliamBergamin
Copy link
Contributor

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 WilliamBergamin added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented good first issue Good for newcomers labels Feb 10, 2023
@funtaps
Copy link
Contributor Author

funtaps commented Feb 10, 2023

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 ?

@seratch
Copy link
Member

seratch commented Feb 21, 2023

@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.

@funtaps
Copy link
Contributor Author

funtaps commented Mar 5, 2023

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.

@github-actions
Copy link

👋 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.

@cbullard-dev
Copy link

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

@seratch seratch added enhancement M-T: A feature request for new functionality discussion M-T: An issue where more input is needed to reach a decision TypeScript-specific auto-triage-skip Prevent this issue from being closed due to lack of activity and removed bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented auto-triage-stale labels Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-triage-skip Prevent this issue from being closed due to lack of activity discussion M-T: An issue where more input is needed to reach a decision enhancement M-T: A feature request for new functionality good first issue Good for newcomers TypeScript-specific
Projects
None yet
Development

No branches or pull requests

4 participants