Response middleware #1021

bhuga · 2015-08-06T19:25:47Z

Hot on the heels of #1019, response middleware gives you the chance to edit outgoing messages.

It looks like this:

# Replace markdown links with slack's <url|text> format.
module.exports = (robot) ->
  robot.responseMiddleware (context, next, done) ->
    regex = /\[([^\[\]]*?)\]\((https?:\/\/.*?)\)/g

    context.string = context.string.replace(regex, "<$2|$1>")
    next(done)

technicalpickles · 2015-08-11T22:06:09Z

By calling robot.adapter.whatever multiple times, rather than passing it an array of strings..., I have a feeling this is going to break some things. The thing that comes to mind is calling response.send multiple times causing messages to come out in random order for the campfire adapter, where response.send with multiple arguments comes out in the correct order.

# Conflicts: # src/robot.coffee # test/robot_test.coffee

bhuga · 2015-08-31T21:19:01Z

Okay, with #1019 landed, I picked this back up.

By calling robot.adapter.whatever multiple times, rather than passing it an array of strings..., I have a feeling this is going to break some things.

I adjusted this to send the list of strings to each middleware instead of each string. It's slightly more cumbersome on the middleware, but 100% backwards compatible.

I also added some docs and a couple of tests. Ready for 👀 again.

michaelansel · 2015-08-31T23:36:39Z

We probably also need a suite of tests for message sending pre-middleware. Not much there right now to protect the existing state.

bhuga · 2015-09-01T13:14:01Z

We probably also need a suite of tests for message sending pre-middleware. Not much there right now to protect the existing state.

This is tough, because hubot-mock-adapter does not actually obey the string, string, string, function() semantics (though now that I look at it, neither does the shell adapter). I'm afraid the different adapter behaviors make the current behavior not terribly well-defined.

michaelansel · 2015-09-01T18:59:15Z

😭 Okay, I guess we just leave the Response/Adapter interface as it is right now (a crazy mess) and put our hopes and dreams on #1036.

bhuga · 2015-09-08T21:09:58Z

Okay, I guess we just leave the Response/Adapter interface as it is right now (a crazy mess) and put our hopes and dreams on #1036.

Singing songs of #1036...

Would this interfere with that? I don't think it would; can we move forward on this separately?

michaelansel · 2015-09-08T21:27:50Z

Yeah, I think this can move forward separately. As long as this is bound to the normal response interface (all the existing function calls), we can ensure backwards compatibility with all manner of shim layers in the core. It ain't gonna be pretty, but it does allow us to segment out the decision making.

technicalpickles · 2015-09-09T16:06:36Z

As an author of response middleware, do you think you'd need to be able to target specific Response methods? For example, would you want to be modify topics vs send and reply differently? I suspect you would, so that would be a good thing to include in the context.

bhuga · 2015-09-09T16:09:06Z

I suspect you would, so that would be a good thing to include in the context.

Really good point! Do you think just context.method would be okay?

bhuga · 2015-09-10T22:14:57Z

I added context.method as the string representation of the method, so send, reply, etc.

michaelansel · 2015-09-10T23:19:36Z

How does this look after #1036 merges and we port all the existing methods over to a singular flow (similar to how Message encompasses all kinds of input)?

michaelansel · 2015-09-15T20:36:13Z

Edit: (completely forgot I had already commented on this and not refreshed. lolwhoops)

My first reaction is concern, but I think this will be okay when mixed in with #1036 as long as the context.method value doesn't change. This has a similar feel to #949 (using named object types instead of inheritance). Without spending time thinking through the long-term design of #1036, I'm not really sure how this will fit, but I think it is okay???

@bhuga, can you try to rough out what this would look like when merged with the ideas in #1036? (i.e. custom prompts, sendImage, sendSticker, etc.) Should we make the response message types more abstract? (e.g. send/reply are both text responses, just like hear/respond are both text listeners)

#michaelneedsmoretimeforthis 😭

bhuga · 2015-09-15T20:46:42Z

@bhuga, can you try to rough out what this would look like when merged with the ideas in #1036? (i.e. custom prompts, sendImage, sendSticker, etc.) Should we make the response message types more abstract? (e.g. send/reply are both text responses, just like hear/respond are both text listeners)

I'll noodle on this. We're running into this sort of thing too, and it's plain that a pile of strings is not going to cut it.

We should also decide if the callback as the last argument to send and friends is something we support or not.

bhuga · 2015-09-16T18:55:01Z

@bhuga, can you try to rough out what this would look like when merged with the ideas in #1036? (i.e. custom prompts, sendImage, sendSticker, etc.) Should we make the response message types more abstract? (e.g. send/reply are both text responses, just like hear/respond are both text listeners)

I thought on this some. I don't think we should make them more abstract. My term strings is wrong.

Different chat systems use structured data to handle messages. I don't think it's possible for a generic middleware to understand all of this structured data, or to provide an interface to make it "plaintext enough". If a script returns structured data, we pass that through to this middleware. So I would rename strings to, say, messages, but I think it's still fine to have a method and a list of data objects.

But I don't see #1036 coming up with something that wouldn't be able to fit that pattern. The method might become keyboard for telegram, and middleware might receive structured data and need to know how to handle it. But I think this response middleware interface would work.

bhuga · 2015-09-23T19:10:03Z

Any objections to merging? Would love to get starting using this.

technicalpickles · 2015-09-29T21:18:43Z

I was thinking about how to deal with adapter's having different maximum lengths of message they support sending, and it occurred to me that response middleware could be useful for that.

Currently, if you send a large message with most adapters, they silently truncate, or the service itself truncates on their behalf. As a developer of a hubot, you wouldn't know that. I was thinking one approach to that would be to have a response middleware that checks the length of a message, and it could do something like emit an error so the developers could track down what was generating large messages.

michaelansel · 2015-10-02T16:41:47Z

docs/scripting.md

+  - `strings`
+    - An array of strings being sent to the chat room adapter. You can edit these, or use `context.strings = ["new strings"]` to replace them.
+  - `method`
+    - A string representing which type of response message the listener sent, such as `send`, `reply`, `emote` or `topic`.


Should this instead be a message type instead of the method name? I think it is better to specify if message.type == text instead of `if message.type in ['send','reply'], but that depends on whether we want to allow differentiation between those types of messages... Maybe not worth worrying about?

Good thought, I hadn't considered that.

Types might be better in some cases, but the edges are fuzzier. Internally we're abstracting away colors in slack attachments, for example, and I would want to consider a helper for that as "text". But is an emote text? Is a topic? What type would a slack attachment, a sent file, or telegram buttons be?

My first instinct is that type would be fuzzy for some of the more basic messages, and have a 1:1 correlation with advanced message formats in specific adapters. It's thus worse than send in the base case and gains no value in the adapter-specific case, instead being a piece of metadata boilerplate that custom message types require without much benefit.

But I'm not really sure. Can you think of any complex messages that could safely share the same type?

I think you're right here. I want there to be a way to group and abstract outgoing messages, but maybe it isn't so easy. I'm really just squeamish about seeing this everywhere: if message.type in ['send','reply']. Can you think of another way to make that extremely common case less gross?

Hm. message.plaintext? Adapters could set it alongside basic send and reply, and it arguably applies to topic and emote, too.

Missed this, but independently had the exact same idea. Thinking in the same style as Ruby message.plaintext? returns a boolean. No ? in CS though, so I like just having it as a "read-only" property. Thinking about applying the same idea to inbound messages to make robot.listen easier to work with (instead of needing to do message instanceof TextMessage every time)

I adopted the context.plaintext pattern. See what you think. f0acd0f...0544029 has the diff from the last time you looked until now.

MrSaints · 2015-10-27T10:41:01Z

I wrote a really simple, and hacky response middleware a while back.

I much prefer this PR though. I'm really glad this is being done, and hopefully it'll be merged soon 👍

michaelansel · 2015-11-12T00:59:35Z

👍 I like it. I think we can probably merge this.

bhuga · 2015-11-12T20:29:12Z

The triumvirate of middleware is complete. Half life 3 confirmed.

Response middleware

Ben Lavender added 2 commits August 6, 2015 14:00

Add response middleware for send

88d5524

Run middleware for emote, topic, play, etc

ea1de9c

This was referenced Aug 11, 2015

Add a delay to responses #1023

Closed

Receive middleware #1019

Merged

Ben Lavender added 3 commits August 31, 2015 10:06

Merge branch 'master' into response-middleware

0d3a0dd

# Conflicts: # src/robot.coffee # test/robot_test.coffee

Allow context.strings =

2e31dac

first stab at docs

9558a99

Ben Lavender added 2 commits September 10, 2015 17:13

Add the method to response middleware

6ba5d1a

Document the response method attachment

9eb00e9

michaelansel reviewed Oct 2, 2015
View reviewed changes

Ben Lavender added 3 commits November 10, 2015 19:02

Merge branch 'master' into response-middleware

202bd86

Fix a bug with array results

f0acd0f

Use splat arguments as we did in olden tymes

7d32855

Ben Lavender added 2 commits November 11, 2015 16:40

Mark plaintext as plaintext

f1b671c

Document context.plaintext

0544029

bhuga pushed a commit that referenced this pull request Nov 12, 2015

Merge pull request #1021 from github/response-middleware

f8506cd

Response middleware

bhuga merged commit f8506cd into master Nov 12, 2015

bhuga deleted the response-middleware branch November 12, 2015 20:29

MrSaints mentioned this pull request Nov 12, 2015

Overhaul to support response middleware ClaudeBot/hubot-longtext#2

Closed

chen-ye mentioned this pull request May 2, 2016

add auto webhook and subscription setup chen-ye/hubot-fb#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Response middleware #1021

Response middleware #1021

bhuga commented Aug 6, 2015

technicalpickles commented Aug 11, 2015

bhuga commented Aug 31, 2015

michaelansel commented Aug 31, 2015

bhuga commented Sep 1, 2015

michaelansel commented Sep 1, 2015

bhuga commented Sep 8, 2015

michaelansel commented Sep 8, 2015

technicalpickles commented Sep 9, 2015

bhuga commented Sep 9, 2015

bhuga commented Sep 10, 2015

michaelansel commented Sep 10, 2015

michaelansel commented Sep 15, 2015

bhuga commented Sep 15, 2015

bhuga commented Sep 16, 2015

bhuga commented Sep 23, 2015

technicalpickles commented Sep 29, 2015

michaelansel Oct 2, 2015

bhuga Oct 2, 2015

michaelansel Oct 27, 2015

bhuga Nov 3, 2015

michaelansel Nov 7, 2015

bhuga Nov 11, 2015

MrSaints commented Oct 27, 2015

michaelansel commented Nov 12, 2015

bhuga commented Nov 12, 2015

Response middleware #1021

Response middleware #1021

Conversation

bhuga commented Aug 6, 2015

technicalpickles commented Aug 11, 2015

bhuga commented Aug 31, 2015

michaelansel commented Aug 31, 2015

bhuga commented Sep 1, 2015

michaelansel commented Sep 1, 2015

bhuga commented Sep 8, 2015

michaelansel commented Sep 8, 2015

technicalpickles commented Sep 9, 2015

bhuga commented Sep 9, 2015

bhuga commented Sep 10, 2015

michaelansel commented Sep 10, 2015

michaelansel commented Sep 15, 2015

bhuga commented Sep 15, 2015

bhuga commented Sep 16, 2015

bhuga commented Sep 23, 2015

technicalpickles commented Sep 29, 2015

michaelansel Oct 2, 2015

Choose a reason for hiding this comment

bhuga Oct 2, 2015

Choose a reason for hiding this comment

michaelansel Oct 27, 2015

Choose a reason for hiding this comment

bhuga Nov 3, 2015

Choose a reason for hiding this comment

michaelansel Nov 7, 2015

Choose a reason for hiding this comment

bhuga Nov 11, 2015

Choose a reason for hiding this comment

MrSaints commented Oct 27, 2015

michaelansel commented Nov 12, 2015

bhuga commented Nov 12, 2015