Forward errors along to error handling middlewares#11
Conversation
arjansingh
force-pushed
the
resilient-error-handling
branch
from
cd18df2 to
052ef84
Compare
|
Looks like this is an alternative to #4. |
|
Note to self, write test to check what happens if we have no error handling middleware. |
arjansingh
force-pushed
the
resilient-error-handling
branch
from
052ef84 to
b975bb3
Compare
|
@tomdale Updated test cases to include testing error propagation when there is no custom error handling middleware. Can you double check the cases for accuracy? I'm happy to add any more that you can think of as well. |
|
|
| console.log(error.stack); | ||
| log(500, error.stack); | ||
| next(error); | ||
| res.sendStatus(500); |
There was a problem hiding this comment.
Should we res.status here instead, so that middleware can alter the response, instead of being forced to deal with sendStatus
arjansingh
force-pushed
the
resilient-error-handling
branch
from
91e3c13 to
5cbeaf5
Compare
|
@danmcclain @tomdale Look it over one more time? |
| res.sendStatus(500); | ||
| } | ||
| if (error.name !== "UnrecognizedURLError") { | ||
| res.status(500); |
There was a problem hiding this comment.
Do we even need to do this? Or would the default handler do this?
There was a problem hiding this comment.
I think we should set 500 because that accurately reflects the state of the request when it leaves the FastBoot middleware.
If post-FastBoot middleware wants to change the status code and attempt recovery it can. But that doesn't change the fact that our middleware resulted in an internal server error (i.e., a 500).
There was a problem hiding this comment.
@arjansingh That rationale sounds reasonable to me.
arjansingh
force-pushed
the
resilient-error-handling
branch
from
5cbeaf5 to
28e3ee2
Compare
|
@danmcclain @tomdale Bueller? 😁 |
|
Thanks @arjansingh 👏 |
3 participants
This would address issues discussed in: ember-fastboot/fastboot#76
Relevant express documentation: https://expressjs.com/en/guide/error-handling.html