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

Use express-boom-v2 #471

Closed
komninoschatzipapas opened this issue Apr 8, 2021 · 2 comments · Fixed by #480
Closed

Use express-boom-v2 #471

komninoschatzipapas opened this issue Apr 8, 2021 · 2 comments · Fixed by #480

Comments

@komninoschatzipapas
Copy link
Contributor

komninoschatzipapas commented Apr 8, 2021

We currently use the @hapi/boom package directly for our HTTP errors. I propose we instead use this package which wraps @hapi/boom as an express middleware.

Doing so will result in more testable and clearer code:

throw Boom.badImplementation('SMTP settings unavailable') // Uses throw and weird global variable, bad practice.

vs.

return res.boom.badImplementation('SMTP settings unavailable') // 🚀
@komninoschatzipapas
Copy link
Contributor Author

Furthermore, Boom errors are sometimes used outside of routes, e.g. on jwt.ts:

if (!jwtKey) {
  throw Boom.badImplementation('Empty JWT secret key.')
}

We should just throw regular Errors in cases like the above.

@elitan
Copy link
Contributor

elitan commented Apr 8, 2021

I like your suggestion and the syntax of returning the http error instead 👍 . Maybe @plmercereau wants to weigh in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants