The anyMessage() function is misleading

[zhawtof]

Context

According to the following code, AnyMessageEvent can be any of the following message events. https://github.com/seratch/slack-edge/blob/05eecb8cafeeed7ef648671ba197b20ad8115bac/src/request/payload/event.ts#L1174-L1191

As part of isPostedMessageEvent, the only messages that are allowed are the 4 types:

• GenericMessageEvent
• BotMessageEvent
• FileShareMessageEvent
• ThreadBroadcastMessageEvent https://github.com/seratch/slack-edge/blob/05eecb8cafeeed7ef648671ba197b20ad8115bac/src/utility/message-events.ts#L8-L22

The Problem

The issue is that isPostedMessageEvent is part of the critical path of .anyMessage() which now only executes the function for those 4 subtypes of messages. https://github.com/seratch/slack-edge/blob/05eecb8cafeeed7ef648671ba197b20ad8115bac/src/app.ts#L271-L286

This is misleading as one would assume the AnyMessageEvent and anyMessage() functions would support the same message types.

Recommendations

1. Remove isPostedMessageEvent from the message() logic. Unless I'm missing something, this seems like the obvious move to conform with the current Slack Web API
2. Provide an additional function to filter all message events by subtype. Something along the lines of:

   app.messageEvent('message_changed', lazyHandler())

   I would also recommend renaming app.message() and app.anyMessage() to app.postedMessage() and app.anyPostedMessage() respectively

3. The minimum would be to update the documentation to mention other message types (e.g. message_changed, channel_topic) should use an EventLazyHandler<'message'> for handling.

Thanks

Huge fan of the framework Kaz. We've done a lot of work migrating over to Cloudflare, but this one caught us up during the transition.

[seratch]

Hi @zhawtof, thanks for sharing this.

The app.message(constraints) and app.anyMessage() are intentionally designed in the way because most developers want to do something only when receiving a new message within a channel. As one of the maintainers of bolt-js, I aim to change bolt-js's behavior it in the future (see https://github.com/slackapi/bolt-js/pull/1801). For this reason, this framework followed the bolt-python's behavior instead.

For your use case, you can use app.event("message", listener) instead. If you prefer switch statement with the subtype, it works well too.

app.event("message", async({ payload }) => { // the payload here is union type
  if (payload.subtype === "message_changed") {
    // inside this if statement, the type of payload is MessageChangedEvent
  }
});

The app.messageEvent('message_changed', ...) approach you suggest could be indeed beneficial for some use cases like yours. However, I hesitate to add the method at this moment. Having a single message listener with dispatching logic inside it like I suggested above is not bad. Also, I'd like to keep this new framework as simple as possible.

I hope you could understand this.

[zhawtof]

Thanks for the quick response!

It makes a lot of sense, and yes we did end up going with the app.event("message", listener) approach with the subtypes switch statements as that was how our original function was built out in Bolt. I am excited about the changes you are planning to make to Bolt as I agree it would greatly help the DevEx.

Last Question

My final question is tied to the AnyMessageEvent type.

Based on what you said above, should this type consist of GenericMessageEvent | BotMessageEvent | FileShareMessageEvent | ThreadBroadcastMessageEvent as that is how slack-edge operates?

The anyMessage() function handles 4 message types whereas AnyMessageEvent consists of 15+ message types. The naming seems to be the problem here as it implies they work alongside each other, but they do not as you stated.

Anyway, I have found my path forward, but I'm sure others might run into this in the future when they migrate from Bolt so wanted to call it out.

[seratch]

AnyXXX is a naming convention that I've been using for this project and its underlying slack-web-api-client. Indeed, the overlap of the names AnyMessageEvent and app.anyMessage() might be sometimes confusing, as you experienced. However, I'd refrain from changing the name just for the reason, as it would bring inconsistencies on the type naming side.

If someone needs a new type representing the union type of the app.anyMessage()'s payload argument, adding such may be worth considering. Having said that, most developers just need MessageEventLazyHandler<ENV> type for defining their code and it's a good thing that the type is already available. For this reason, I don't think this library should expose the union type to users at least for now.

Thank you so much again for sharing valuable feedback here! Since I don't have anything else to share/discuss here, let me close this issue now. That said, please feel free to write in whenever you have more to ask on this topic.