Handling promises at router layer #65
Conversation
|
Is this a duplicate of #47 ? |
|
Yes, I did not see it. Close if you want or tell me what PR is better in terms of code. |
|
Ok. I'll review through your code to understand what it does. If you can provide highlights of the differences between the two PRs, that would be super helpful as well 👍 On the surface it seems the main difference is that #47 passes CI and this PR doesn't pass the CI. |
|
I think my code is simpliest and more readable. That's true, I have not passed the CI. But I don't know why, because I passed tests ( |
|
What is the behavior of this code for a promise that is rejected without a value (a valid rejection)? i.e. calling |
|
Actually, an incorrect behavior. Fixing... |
|
This error comes from 2.0 branch. That's why builds are failing. |
There was a problem hiding this comment.
Can you look at these comments? Also the code looks almost identical to #47 I'm not sure it would be fair to that author to accept this PR over the work they put into their PR especially when we already marked their PR to merge into 2.0. Why should we reject the work put into #47 and accept this instead? Can you work with the other author to come to an agreement here? I won't merge either one until there is some agreement between you two on which I should land.
| }) | ||
|
|
||
| router.use(function (err, req, res, next) { | ||
| res.sendStatus(500) |
There was a problem hiding this comment.
There is no such thing as res.sendStatus
| }) | ||
|
|
||
| router.use(function (err, req, res, next) { | ||
| res.sendStatus(500) |
There was a problem hiding this comment.
There is no such thing as res.sendStatus
| @@ -124,6 +124,92 @@ describe('Router', function () { | |||
| .expect(404, cb) | |||
| }) | |||
|
|
|||
| it('should support promise as route handler', function (done) { | |||
There was a problem hiding this comment.
What does this test? I ran this test without any of your changes and it passed. This test doesn't seem to actually depend on any change, or maybe I'm just missing something.
| }) | ||
|
|
||
| router.use(function (err, req, res, next) { | ||
| res.sendStatus(500) |
There was a problem hiding this comment.
There is no such thing as res.sendStatus
|
Ok, I think both PR have same functionality then Imy PR doesn't add value. As you said, the other author have spent his time in development. I expect that you release 2.0 version soon. Thank you for all. :) |
This feature will avoid try/catch block in all express controllers with async functions (async/await) or functions that returns a promise