message_received event is not a catch-all

[blaenk]

(I was writing this as I investigated the issue, so it became a bit long. Thanks for creating botkit!)

Problem

I wanted to hook onto 'message_received' but I noticed that my handler isn't firing at all.

1. It seems to happen on an rtm message here, as I would expect
2. it's correctly triggering the event here
3. could it be this?
4. this is the "default handler" that is registered by botkit

Actually yeah, it looks like the default handler at #4 will run first given the definition of trigger(), and then what I said in #3, this, prevents subsequent handlers from running if a handler returns false. It seems that there are a variety of situations in which the default handler may return false.

For reference, I'm simply typing a simple message such as "blah". Indeed, this would be considered an "ambient" message and the default handler would mark it as such and trigger "ambient" and return false, cutting off all subsequent "message_received" handlers.

This is confusing because the event is referred to as a catch-all:

This event is fired for any message of any kind that is received and can be used as a catch all

Though it's not much of a catch all when the default handler catches most cases and cuts off subsequent handlers.

Considerations

1. Can we perhaps reconsider the role of the default handler, so that it allows other handlers to run?

2. More generally, could we reconsider the idea that any random handler may prevent others from running?

   Personally it goes against the whole idea of event handlers for me, where you register one without having to worry about what other handlers might do. All it takes is one faulty handler to accidentally return false in some edge case to bring the whole thing down.

   Is this functionality the reason why we're not using something more standard like eventemitter?

   In my opinion, if we want some sort of pipeline/stream API where we can hook into separate phases/stages of the pipeline to transform the message/input successively and have the result affect subsequent stages, that could be interesting. However, in my opinion, that would be better served as a separate API instead of clobbering the simple event pattern that most people are already familiar with.

3. There are two useful interpretations of "message_received" that I can think of:

   1. High-level: The equivalent of ["ambient", "direct_mention", "mention", "direct_message"].
   2. Low-level: The raw message as received via RTM

   In my opinion both would be useful. The first as a convenience (and to match the readme), and the second to benefit from the JSON parsing (and possible future work) that botkit already does with the raw messages, which would be lost if we were to simply directly access the controller.rtm.on("message"), if that even works.

   Recommendation

My recommendation would be to make "message_received" an alias for ["ambient", "direct_mention", "mention", "direct_message"], and introduce something like "rtm_message" to the websocket events which would happen directly after the message is JSON parsed.

[RafaelCosman]

Is this functionality the reason why we're not using something more standard like eventemitter?

I don't see any reason not to use something more standard. I would love to see a pull-request! @benbrown, do you know any reason not to use something more standard as recommended here?

[RafaelCosman]

My recommendation would be...

Good recommendation. If anyone wants to make a pull-request with this I'd love to see that!

[peterswimm]

Closing the issue for now, If you have any updates, or if anyone wants to submit a PR, please feel free to reopen!