Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: Make middleware pipeline async with promises #238

Closed
wants to merge 1 commit into from

Conversation

deslee
Copy link

@deslee deslee commented Aug 15, 2019

Summary

To address some of the problems with hosting bolt as a serverless function (#220)

For example, in AWS Lambda, we must not call res.end() until the very end of our flow. This is due to https calls forcefully terminating after we ack with a response. The middleware pipeline needs to be async in order for us to know when bolt is finished with the event.

Another issue I haven't addressed in this PR is the fact that the receiver is using Node's EventEmitter class. The emit method does not return a promise or take a callback, so we won't be able to know when the serverless function can ack. My personal solution was to write a custom receiver using emittery, which provides an emitter that returns a promise. I feel like this is a bad practice however, and there is probably a better way to process an event in a way that's safe to run inside a serverless handler.

Using the changes in my forked repo, I was able to get a bolt-powered endpoint working in Zeit Now, which is powered by AWS Lambda.

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2019

CLA assistant check
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.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #238 into master will not change coverage.
The diff coverage is 52.17%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #238   +/-   ##
======================================
  Coverage    62.5%   62.5%           
======================================
  Files           7       7           
  Lines         464     464           
  Branches      126     126           
======================================
  Hits          290     290           
  Misses        150     150           
  Partials       24      24
Impacted Files Coverage Δ
src/ExpressReceiver.ts 51.42% <0%> (ø) ⬆️
src/conversation-store.ts 100% <100%> (ø) ⬆️
src/middleware/builtin.ts 39.82% <45.45%> (ø) ⬆️
src/App.ts 73.2% <50%> (ø) ⬆️
src/middleware/process.ts 56.25% <58.33%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 522e70b...618b2fa. Read the comment docs.

tsub added a commit to tsub/serverless-daily-standup-bot that referenced this pull request Sep 7, 2019
@seratch
Copy link
Member

seratch commented Jun 8, 2020

@deslee Thank you very much for taking the time to make this PR. Also, I'm very sorry for our belated response. 🙇

As you may be already aware, Bolt v2 has resolved the issue you mentioned here in a quite similar way. The latest version supports async functions for middleware/listeners.

So, please allow me to close this PR now. If you have feedback/suggestions on Bolt v2, we'd love to know them in a new GitHub issue.

@seratch seratch closed this Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants