davidbanham / express-async-errors

async/await support for ExpressJS
Other
900 stars 43 forks source link

Add two more express functionalities #21

Closed awreccan closed 5 years ago

awreccan commented 5 years ago

As such, express-async-errors breaks certain express functionality that regular users are used to. This pull request aims to fill some of those gaps.

Tests

Added 3 more tests for the new functionality image

awreccan commented 5 years ago

@davidbanham my team is rather interested in using this library, but we'll need it to support other express functionalities as well. Would you be interested in considering this pull request? 🙂

kronicker commented 5 years ago

Hi @awreccan, thanks for the interest in this library and your contribution.

So I would like to first address your requests before we get into code review.

Your first issue is that this library doesn't automatically call next in case of resolved promise claiming (in #20 ) it's native to express. But, you can chain middlewares as you would without this library included. It's not native to express to automagically call next at the end of the middleware, what's more, if you don't call it, the request will hang and the next middleware will never be invoked. Also, I think a library called express-async-errors shouldn't handle non-error cases and I'm not eager to expand its scope to areas which it wasn't meant to cover.

Your second issue is that this library doesn't support next('route') case while it actually does. You can call next('route') and it will behave as it is expected. 'route' argument in this special case is a String, not an Error. If you call next(new Error('route')) express will pass it to the error handler not skip next middleware. So if we would support what you're asking we would be altering expected express behaviour.

@davidbanham You can rule in if you'd like, but I'm not in favor of accepting these feature requests.

vladimyr commented 5 years ago

@davidbanham my team is rather interested in using this library, but we'll need it to support other express functionalities as well.

This library extends express by automatic async error capturing without altering its core behavior; so would you please explain what additional features of express it does not/you need to support? 🤔

As per PR itself:

  1. My advice to you is to open issue next time instead of jumping right into your code editor. Asking right questions prevents wasted efforts...
  2. @kronicker said it all and I wholeheartedly agree.
awreccan commented 5 years ago

Agreed guys. I was hoping to replace express's callback-based middleware with async functions but you're right, it can't be done because the callback performs more functions than simply indicating resolution of the middleware. Apologies for the waste of time and thanks for the thoughtful responses.