Let middleware return a promise

[timkinnane]

Hey humans. I keep hitting a logic wall where I need to wait for hubot to finish processing middleware, but the only way to do that currently is to add more middleware, which doesn't work with my requirements. That approach could also be buggy, as you'd still want to resolve the promise even if the first middleware prevents others from executing.

Use case 1:

Unit testing expects res.reply strings to be transformed by middleware.
Currently I have to wait and observe the test helper's messages for a matching pattern, calling the test's done when the reply middleware stack finishes and finally outputs a message, because res.reply returns immediately but the transform happens asynchronously. Very pain.

If middleware.execute returned a promise, it would cascade up through runWithMiddleware and all the response methods that invoke it, like send, emote, reply etc. Much wow.

Then I could very simply do something like this:

it 'capitalises a string', (done) ->
  res.reply 'sent string'
  .then (context) ->
    expect(context.strings).to.equal ['Sent String']
    done()

Use case 2

after a user sends something to hubot, listener middleware stores the match and performs some lookup for correlating data. I need to use that data in the listener callback. I can't move the middleware logic to the callback (because reasons), but I can't reply until I know the middleware has completed.

I've heard that async logic support will be improved in the next version, but this is a pretty small change that will really increase the versatility of my bots, so I'm hoping I can get some help with a PR and get it through without waiting for broader roadmap stuff.

I'll post PR to follow up, but the middleware logic looks like black magic to me and I'm unfamiliar with the async library, so it won't be accepted without feedback. Thanks in advance.

[technicalpickles]

I'd have to pour over the middleware implementation, but I was under the impression that it could be async in its current state (ie without promises). I just checked the kinds of middleware we have, and one of them is 2FA powered by Duo, which definitely has an async call in there, which just calls the next middleware. That is a bit different though than promises.

If middleware.execute returned a promise, it would cascade up through runWithMiddleware and all the response methods that invoke it, like send, emote, reply etc. Much wow.

Wouldn't we also need promises in listeners too? That would address part of #613 and add even more flexibility, ie being able to know exactly when a listener is done, rather than having to set a timeout waiting for a response.

[timkinnane]

Yeah I guess there's two issues, async within middleware, for which there seems to be solutions, but this relates more to asynchronous wrapping of the middleware stack. More context copied from slack, for the record...

I've been solving it in hubot-pretend with underscore-observe, which lets me monitor the messages array and fire a callback when a message appears, which is a good enough equivalent to a promise resolve at the end of the middleware stack. BUT it's fairly hacky and limited, more expensive, less graceful than a promise. And it only solves one of my issues, on the test side. Won't help in production.

Given the example of middleware that transforms a user's message into data that hubot will use in it's reply...

not so simple as getting it from the match however, because they may have been routed to that point in the conversation from different ways, or be calling back a value from a prior interaction. So all I need to know is that before I send the next message, that the middleware receiving the last message has finished transforming and storing the match object into the records it needs. ...even running the brain.set through middleware, then running a follow up res.reply, the message can send before the brain finished receiving the data.

[timkinnane]

re #613 - I can see the need to get a promise state/then for async listeners, e.g. if getting a reply from hubot isn't enough. This change would apply equally to listen as well as respond middleware, but I'm unsure how the specific requirements would deviate and don't want to bite off more than I can chew.

[technicalpickles]

I have been thinking about this a bit, and I think rather than introduce Promises in this one place, we more thoroughly apply Promises to the Hubot architecture. We actually have a place to discuss that now, with the Hubot Evolution process

[timkinnane]

Bummer, if I had more time I could have finished the PR already before you had time to reconsider (just finished anyway, in case there's a change of heart) :P

I would definitely agree to the larger goal of comprehensive async architecture, but I don't see the harm in this light-touch solution in the mean time, that solves some significant issues without much change and zero effect of compatibility.

If I'm the only one that sees it that way though, I'll just have to maintain the fork because its a pretty critical requirement for me and won't wait for the v3 roadmap to play out.

[gr2m]

@timkinnane I hear you, I didn’t get to it yet but will give it a thorough review myself and if at all possible I’ll try to help release a new Hubot 2.x version, we need a workflow for that anyway in case of security bugs. I’ll keep you posted, still catching up with all things Hubot and working on the CoffeeScript ➡️ JavaScript evolution proposal, for which your issue/PR is relevant, too

[timkinnane]

OK thanks @gr2m, I appreciate this still being considered whenever it makes most sense. In the mean time I'll look at @technicalpickles suggestion of assessing all other possible async/promise returns to implement a consistent approach across the board.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[timkinnane]

Not stale. Shoo bot.

[joeyguerra]

the async module was replaced with async/await code. Closing this issue. If it doesn't address the issue, please open another Github issue.