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

Discussion: Improvements around app.message(...)

Open seratch opened this issue 5 years ago • 11 comments

Proposal

As described at #463, app.message(...)'s behavior with the latest payloads is not intuitive for developers. It's surprising for developers to receive more events than the number of messages in a channel.

I propose to change the behavior to receive only message typed events that don't have a subtype. With this, Bolt apps can receive only events telling the arrival of new messages.

Also. there is one additional thing we may need to care about in the future. As far as I know, the server-side no longer sends bot_message subtyped events. Instead, it sends no-subtype events with bot_id and bot_profile properties. If the server-side gets back to the previous behavior in the future, we may need to add bot_message events as well.

For reference: @aoberoi 's https://github.com/slackapi/hubot-slack/issues/589

What happened to the subtype: 'bot_message'? It seems to be missing on at least some, but maybe all, events where it should be.

References

The list of sutyped message events

There are a number of subtype events for messages. Some of them may have a quite different data structure compared with message typed events without a subtype.

  • bot_message
  • ekm_access_denied
  • me_message
  • message_changed
  • message_deleted
  • message_replied
  • reply_broadcast
  • thread_broadcast

Bolt for Java's approach

Bolt for Java's app.message listeners receive only message events that don't have a subtype. https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt/src/main/java/com/slack/api/bolt/App.java#L509-L526

There are two reasons for that. It's technically impossible to include other events due to the Reason 2.

  • Reason 1: @seratch believes developers expect to receive only new incoming messages when they use keyword matching in text
  • Reason 2: It's not possible to support completely different structure in a statically-typed language (we're already aware of the same issue with TypeScript https://github.com/slackapi/bolt/issues/311)

Regarding bot_message events, Bolt doesn't invoke listeners given to app.message due to the difference in the structure of payloads. If the server-side is changed in the future, Bolt for Java may need to consider introducing a new routing method.

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

  • [ ] bug
  • [ ] enhancement (feature request)
  • [ ] question
  • [ ] documentation related
  • [ ] testing related
  • [x] 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.

seratch avatar Apr 06 '20 02:04 seratch

This is a well thought out and thorough proposal!

Question: In Bolt for Java, would it be possible to use constraints properties to vary the type of arguments passed to a listener? This is how we've been able to solve this problem in Bolt for JavaScript in the app.event<EventType>() method. I think this is a clean solution that doesn't expand the App API too much, but it might depend on unique capabilities of the TypeScript type system.

aoberoi avatar Apr 13 '20 20:04 aoberoi

Question: In Bolt for Java, would it be possible to use constraints properties to vary the type of arguments passed to a listener?

Yes, it's possible and even is necessary for Bolt for Java apps. Developers are not allowed to make the type ambiguous.

Here is a simple example demonstrating how to use it: https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt-servlet/src/test/java/samples/EventsSample.java#L21-L29 The first one in the listener args is typed with the given event class like MessageEvent. The type constraint is implemented this way: https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt/src/main/java/com/slack/api/bolt/App.java#L492

seratch avatar Apr 14 '20 00:04 seratch

Thanks for writing this out @seratch!

So the proposal is to have app.message only handle message type events. Sounds fine to me.

Questions:

  1. would this be a breaking change?
  2. Is there any other way for bolt users to still handle events such as message_changed?

stevengill avatar Apr 16 '20 18:04 stevengill

would this be a breaking change?

Good point. Yes, it is a breaking change. So, it may be better to consider a bit careful transition like providing an option to change the behavior in the next minor release, and then changing the default in the next major release.

Is there any other way for bolt users to still handle events such as message_changed?

I think app.event is the best way to handle those. Developers can check what the subtype is inside a listener or middleware.

seratch avatar Apr 16 '20 22:04 seratch

As far as I know, the server-side no longer sends bot_message subtyped events.

This was a wrong assumption. If other bot users in the same workspace are still classic apps, they may generate message events with bot_message subtype.

As of version 1.4.4, bolt-python handles only non-subtyped and "bot_message" subtype events by app.message listeners. This is the behavior most developers should expect. We can revisit this issue and it's worth considering to apply the behavior changes in v4.0 (of course, with a clear mention about it in the migration guide).

seratch avatar Mar 23 '21 03:03 seratch

We can revisit this issue and it's worth considering to apply the behavior changes i

Hey so if I am building a slack app currently and receiving events via the slack events web api should I handle the cases with subtype as bot_message or consider they will not be subtyped with the presence of bot_id and bot_profile properties?

batflarrow avatar May 26 '22 07:05 batflarrow

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

seratch avatar May 26 '22 08:05 seratch

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

It's the other way round right "classic" app's messages with have a sub_type as bot_message and not the new apps this was what you mentioned above in this comment.

batflarrow avatar May 26 '22 08:05 batflarrow

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

It's the other way round right "classic" app's messages with have a sub_type as bot_message and not the new apps?

Also is there a list of these "classic" apps, how can I test my code for cases with sub_type as bot_message or is there a way just to know the payload in cases where the events will be sub_typed as bot_message

batflarrow avatar May 26 '22 08:05 batflarrow

If you don't have so many apps in your workspace, it may not be so hard to review the scopes of the apps listed at https://slack.com/apps/manage . However, I think that your goal is to safely respond to all new messages. If so, all you need to do are:

  • Check if a message event has "bot_message" subtype or does not have any subtype
  • If yes, extract text property from the event data and respond to it

seratch avatar May 26 '22 08:05 seratch

If you don't have so many apps in your workspace, it may not be so hard to review the scopes of the apps listed at https://slack.com/apps/manage . However, I think that your goal is to safely respond to all new messages. If so, all you need to do are:

  • Check if a message event has "bot_message" subtype or does not have any subtype
  • If yes, extract text property from the event data and respond to it

yeah but I am building a slack app myself for people to use and they can have any apps installed on their systems themselves so I will have to handle the cases with the "bot_message" subtype. It would be great if I could find a event with the sub_type as bot_message but anyways I'll look for it .The issue is the I need the bot_profile.app_id of the bot in all cases and is it available in the cases where the event is sub_typed as 'bot_message' Also can you confirm "classic" apps have the subtype right and not the new apps coz you mentioned it the otherway round here and here.

batflarrow avatar May 26 '22 08:05 batflarrow