Add comment explaining default Express error handling #894
Conversation
| @@ -35,7 +35,8 @@ app.use((err, req, res, next) => { | |||
| return res.status(err.statusCode).json({ message: err.message, data: err.data }) | |||
| } | |||
|
|
|||
| return next(err) | |||
| console.error(err); | |||
| return res.status(500).json({message: "An internal server error occurred."}) | |||
There was a problem hiding this comment.
What really confuses me is that I was 100% sure we already had this in place!
Look, even the docs say it: https://wasp-lang.dev/docs/language/features#error-handling .
I looked at older code but we never had explicit handling of this.
So I checked express docs: https://expressjs.com/en/guide/error-handling.html#the-default-error-handler .
So from what I get, by default they will:
- Set status message to 500 if message doesn't have status that is in 400-599 range.
- Set error message to an HTML description of its status if in production, or stack if in dev. So maybe that is why you saw stack, while in production it wouldn't be visible
Based on that, I think it best that we don't touch anything!
But, I do agree this is completely unclear from the code. Maybe we should, above return next(err) line, put a link to this piece in docs and quickly comment what it does?
Here is except from their docs:
If you pass an error to next() and you do not handle it in a custom error handler, it will be handled by the built-in error handler; the error will be written to the client with the stack trace. The stack trace is not included in the production environment.
Set the environment variable NODE_ENV to production, to run the app in production mode.
When an error is written, the following information is added to the response:
The res.statusCode is set from err.status (or err.statusCode). If this value is outside the 4xx or 5xx range, it will be set to 500.
The res.statusMessage is set according to the status code.
The body will be the HTML of the status code message when in production environment, otherwise will be err.stack.
Any headers specified in an err.headers object.
If you call next() with an error after you have started writing the response (for example, if you encounter an error while streaming the response to the client) the Express default error handler closes the connection and fails the request.
There was a problem hiding this comment.
Ah, duh! Ok, thanks. That is embarrassing. I didn't even stop to think it was env-based, which obviously makes sense. The Issue and our chat had me thinking it had to be on us somehow lol but ok yeah, in prod it would work. I will update it with the link to the default error handler, thanks! 👍🏻
There was a problem hiding this comment.
The minor downside is it returns HTML and not JSON, but that is fine for now I think.
shayneczyzewski
changed the title
|
Sorry @shayneczyzewski I thought I wrote a comment but I see now that it actually got rejected by Github (I was sending it via email)! True would be nicer if it was JSON! |
|
No sweat. Thanks for the reply and sorry for my delay (Fly deploy rabbit
hole :D). Will check it out a bit more this week. Thanks!
…On Mon, Jan 2, 2023 at 11:10 AM Martin Šošić ***@***.***> wrote:
Sorry @shayneczyzewski <https://github.com/shayneczyzewski> I thought I
wrote a comment but I see now that it actually got rejected by Github (I
was sending it via email)!
Here it is:
True would be nicer if it was JSON!
And it is a bit confusing that Dev sees one behaviour in production and
another in development, they might not be aware behaviour is different and
get confused. It would probably be better to always return behave like in
prod, but in Dev we just ensure it is logged to console (maybe that is
already happening). Yeah that makes more sense to me than their default
behaviour. Maybe we should do that? What do you think?
—
Reply to this email directly, view it on GitHub
<#894 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD725HQFTXIBU53MOA5EMTWQL4WJANCNFSM6AAAAAATFZA2ZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Description
Currently, if we have any uncaught errors on the server, we are sending those down to the client. This is not great. Example:
This PR updates our custom error handler to return a 500 if it is not an
instanceof HttpError. There was no existing downstream handler/transformer I could find.Aside: The only places I noticed where we do anything related is:
This fix transforms it into:
Fixes #65
Type of change
Please select the option(s) that is more relevant.