Add support for a generic event receiver

[barlock]

Fixes #81

The goal for this is to use Slapp in a serverless environment where there isn't a lot of control over the req object.

[barlock]

FYI: When I ran npm run docs it added a lot of other stuff that was unrelated to this pr. I went ahead and cleared it out for clarity of the commit, but the information in there looked useful and it's probably worth getting those updates in.

[coveralls]

Coverage remained the same at 100.0% when pulling 76b6417 on barlock:generic-receiver into 418f844 on BeepBoopHQ:master.

[selfcontained]

This looks great! I'm curious if you have any additional context around how we might support this bit around using the original response to reply in a serverless setup:

https://github.com/BeepBoopHQ/slapp/pull/83/files#diff-e367f1e8f0e82d72c72718c961f0b97fR16

We originally didn't have this bit, and it would always use the response url to reply to slash commands and message actions, but noticed it makes quite a big difference when you don't get that built in spinner while it waits for the response (hopefully that makes sense). By responding right away and making an api call later you lose out on that built in slack functionality.

Currently it relies on the response object to have a response.send() function, as well as a response.headersSent boolean.

I'd love to hear any thoughts you had around how we might support that, or challenges you've run into around that part. Other than that I think this looks great, just want to flush out what a potential path to adding that functionality might look like so we avoid having to change the public api later.

[barlock]

Absolutely, this was actually an interesting thing that I had to think on. The conclusion that I came to was that I do not believe that this should be a thing supported by Slapp if you use this specific interface.

The way I would end up working this functionality into a serverless context (generically) is by having two different "functions" (action handlers). The one function would receive the post events, call a handler "function" that contains Slapp, wait 2.5s and respond with a 200.

Inside the second function that contains Slapp, it would always call the response_url. I have confirmed with Slack support (and I assume engineers they consulted), though haven't tried in practice, that calling the respose_url before the initial post returns would result in slack's spinner going away and the message updating fine.

What you would essentially get is the default behavoir that slapp has where, the response in Slack's UI spins until you call msg.respond.

BUT, the crappy thing is that it isn't baked into Slapp.

After writing this, I think it probably makes sense to, on events that support it, let it take an optional http.ServerResponse (which is what express uses right?) so that if someone hooks this up to something that isnt express but still happens to have that response object, it can still do it. What do you think?

[selfcontained]

Thanks for sharing your thought process, can see how you got to that conclusion for sure.

Allowing an optional param that is the response object would be pretty useful I think, and if you have it to pass in, you'd get that functionality. If not, it'd fallback nicely.

So express actually adds a few things to http.ServerResponse - one of which Slapp currently uses is response.send() - which sets some headers and handles json stringification.

https://github.com/expressjs/express/blob/master/lib/response.js#L106

I know Google Cloud functions has a send() function on the response, but am not familiar with others. One option could be assuming the response object passed in has a send(), if not, and you wanted the respond behavior, you would have to stub it yourself. Maybe we would want to add some checks in Slapp to make sure there was a send function on the response in order for us to use it too.

I think that could be added w/o adjusting the way you parse the data. The response can be attached in the receiveHandler() by calling msg.attachResponse().

[barlock]

Yeah definitely, sadly the framework I'm most familiar with is OpenWhisk which is easily the most obscure of all of them. I'll look a little harder into lambda and GCF's node runtime to see what they do.

I think the best thing to do would be to allow the optional response, if it has a send take it, if it doesn't ignore it for now. Circle back around to stubbing out send in a different PR sense this one is already a little large. That will keep the api from breaking and let it only get better with time.

[selfcontained]

That sounds good. Did you want to add the optional response and send check to this PR, or in a followup?

[barlock]

I'll do it here, I want to do some digging on those request objects on various platforms

[barlock]

Though, adding an optional parameter won't be a breaking change... Your call if that's the only thing your questioning in this pull. I'm not sure how you roll releases but either way is fine

[selfcontained]

A followup PR would be fine. I'll test this out just to give it a try in a cloud function environment and then I think we can merge it. If you happen to get those other pieces done before I test it out feel free to add them to this PR.

[selfcontained]

One thing I'm noticing is that the context lookup middleware wouldn't run for these messages. That's the piece that asynchronously hydrates the message w/ team specific info, like tokens. Were you doing that manually somewhere? I'm trying this branch out now and will dig in a little to see how we might integrate that functionality.

[selfcontained]

Heads up, #85 has been merged, which tweaks the parse event logic slightly, so will need to be resolved in your branch.

[barlock]

👍 also 🤓🌴, smh, I totally missed the context function.

I don't know how to make this work cleanly with the context function, I can patch it, but it's starting to get a little weird. The reason is that the context function also expects a req and a res with the res in "normal" circumstances being responsible for error handling (are there other use cases for having it there?). This means that on your average case, you wouldn't be able to port your app from express to serverless by just attaching differently, you'd also need to write your context function to be able to handle itself without a res being passed in.

While that's not a big deal. It would be an annoying case for a dev to have to debug when they switched. I would think the cleanest approach would be to have the "serverless recievers" call a context with a different signature context (slappData, callback) to make it more "node like".

Thinking out loud here, you could make a breaking change to Slapp that this is the new ™️ signature for context functions and automatically handle the res.send on an error.

I've pushed my patched version up c843826 so you can see what I mean about it getting a little weird.

[selfcontained]

Yup, totally agree with what you said. Lemme circle with @mbrevoort in the next few days. I kinda feel like there might need to be a more generic context signature like you mentioned, one that doesn't assume http.

[simonsayscode]

If I may chime in here, a few thoughts:

I've been thinking about forking slapp for serverless use too so super happy to see @barlock take a stab at it first. In regards to the context, not only does that need to be adjusted for a non-standard req/res, but also the default express middleware handling should be implemented in the serverless handlers. I agree with the suggestion of a generic context signature, and perhaps even further just moving away from an express req and res object and relying on slapp defined constructs that could provide all the things that are extracted from req and res currently. I imagine something similar to how slapp allows for a stub of the conversation store in order to adapt it to whatever stack you're using.

https://github.com/BeepBoopHQ/slapp/blob/master/src/receiver/index.js#L57
For reference:

      app.post(options.event,
        ParseEvent(),
        sslCheck,
        verifyToken,
        this.context,
        emitHandler
      )

I've also been thinking about how to maybe expand slapp's own middleware to perhaps handle more complex cases (like applying middleware to specific actions, similar to how you can configure express middleware). Perhaps if there was a more slapp configured way of handling middleware instead of relying on express then it would solve both problems?

In regards to the response_url and the spinner, perhaps that there could be some global map of responses to be sent out. It could be identified by maybe the response_url and linked to the
responseTimeout id that was created, and when you reply to an initiated action, it would cancel that timeout. That way you could manage the "response done" behavior on slapp's side as opposed to relying on slack to properly handle a response via response_url followed by a 200.

[barlock]

Totally agree that custom middleware would be useful.

The reason I went the way I did in my pr was because every type of event on every platform would (probably) need to be handled differently. As in, the data sent back in the request would need to take a different format for every environment (lambda, gcf, openwhisk). So in order to make it work well, you'd need to write a handler for every environment (or auto detect it, which would be nasty I think).

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Michael Barlock seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[barlock]

This no longer needs to be open. Something like serverless-http can wrap the app.