Catches errors from Promises / Async functions #47
Conversation
|
Duplicate of #32 and probably a couple of other PRs. If you're going to build this feature, just remember that promises can be rejected with a falsy value and this doesn't handle it right now. |
When returning Promises from a route handler either explicitly or implicitly by using Async Functions,default error handling is lost.
```
// Error caught and forwarded as expected
app.get('/', (req, res) => {
throw new Error('Oh crap!')
})
// Error swallowed and lost
app.get('/', async (req, res) => {
throw new Error('Oh crap!')
})
```
|
Updated to handle falsey rejection as well. I really appreciate the work you put into #32, but my hope is that this is a minimal fix for this specific issue to incrementally introduce working with Promises into router/express while preserving as much existing behavior as possible to reduce migration pain. As @dougwilson pointed out in expressjs/express#3053, with this PR the only major detail library authors will need to consider is that if they wrap middleware or route handlers that they return the result of calling the original middleware/handler. |
This is a great point, and I realized a similar issue currently exists for throwing falsey values which is something that's possible but pretty uncommon. Let me know if you would like me to open an issue for that or approach a fix. |
|
Let me know if there are other concerns - I'm happy to add more tests or add other mitigations. |
|
Just revisiting this since it's been a month or so - any evolution of thinking on this? Other concerns to address? |
|
Bumppp... |
|
@dougwilson Is any help needed here? |
|
I'm not sure if it's helpful or not, but I've been using an outside implementation of this in production for awhile now: https://github.com/jmar777/node-async-router. If nothing else, it might be useful to @olalonde, if you were looking for a similar solution. It handles the error scenario mentioned by the OP (relevant test case). |
|
What's the status of this? Are there any plans to pull this in? @blakeembrey |
|
LGTM :) |
|
This PR is going to land in the new 2.0 branch and release as an alpha version for preview this upcoming week. |
|
Sorry for the delay. I just rebased the |
|
I have written up some docs for this PR now (it had no docs). Now just adding more tests because the tests are just on a tiny part of the API surface, but promises are being handled with this PR on a large part of the API so needs the tests added to all those areas. |
|
Alright, I got this committed to the |
|
Published to npm as version |
When returning Promises from a route handler either explicitly or implicitly by using Async Functions, default error handling is lost.
This PR began as expressjs/express#3053, however it was a breaking change and Route/Layer have been moved to
routerfor express@5.