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

Properly handle async errors #384

Merged
merged 3 commits into from Jan 12, 2018
Merged

Properly handle async errors #384

merged 3 commits into from Jan 12, 2018

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Jan 3, 2018

Express is not optimized for async/await, so async errors are currently bubbled up to the main process and not properly handled. You can read more about Express error handling here:
http://thecodebarbarian.com/80-20-guide-to-express-error-handling

Since Probot is so async/await heavy, I think it makes sense to pull in this extension that properly handles rejected promises in Express.

Express is not optimized for async/await, so async errors are currently
bubbled up to the main process and not properly handled. You can read
more about Express error handling here:
http://thecodebarbarian.com/80-20-guide-to-express-error-handling

Since Probot is so async/await heavy, I think it makes sense to pull in
this extension that properly handles rejected promises in Express.
@codecov
Copy link

codecov bot commented Jan 3, 2018

Codecov Report

Merging #384 into master will decrease coverage by 1%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
- Coverage   95.03%   94.02%   -1.01%     
==========================================
  Files          18       19       +1     
  Lines         262      268       +6     
  Branches       30       30              
==========================================
+ Hits          249      252       +3     
- Misses         11       14       +3     
  Partials        2        2

@bkeepers bkeepers requested a review from a team January 4, 2018 15:17
@bkeepers
Copy link
Contributor Author

bkeepers commented Jan 4, 2018

Using this branch on the project I'm working on and it's helped expose a bunch of issues where exceptions were being thrown in async code.

Copy link
Member

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - it's a cute little middleware

Copy link
Contributor

@tcbyrd tcbyrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. I took a look at the library and it was pretty easy to grok what's happening. I would hope this eventually makes it into express natively, or we eventually migrate to koajs, since that more naturally works with async/await.

@wilhelmklopp
Copy link
Contributor

@bkeepers bump to merge this, as we currently can't use new probot features since our project's probot dependency is pinned to this branch

* origin/master:
  Customize Codecov ☂️ (#386)
  Remove localtunnel (#387)
  Use smee-client instead of EventSource directly (#389)
  Fix lint error introduced in #390
  Document probot-config extension (#390)
  feature: Add early termination to paginate method (#329)
  docs: Update "Remix on Glitch" button link (#388)
  docs: How to install Node and Probot for Development (#381)
  [Suggestion] Use marketplace link instread of original link (#383)
@bkeepers bkeepers merged commit a3c91af into master Jan 12, 2018
@bkeepers bkeepers deleted the async-errors branch January 12, 2018 14:10
@bkeepers
Copy link
Contributor Author

I would hope this eventually makes it into express natively, or we eventually migrate to koajs, since that more naturally works with async/await.

I would be up for that. I haven't used koa, but definitely seems like a better fit.

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.

None yet

4 participants