-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: unify server error response across interceptors #555
Conversation
Hi, @mikicho 👋 If you have a minute, can I ask for your opinion on this? This is a bit related to handling response stream errors. I'd like to hear your thoughts on this exception handling and if it makes sense to you. Thanks! |
if (resolverResult.error instanceof Error) { | ||
// Treat unhandled exceptions in the "request" listener | ||
// as 500 server errors. | ||
xhrRequestController.respondWith( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument of treating exceptions as 500 responses is good, see: #516. TL;DR, if exceptions are preserved, they will be handled by the higher request client. Not only different clients handle those exceptions differently, but that harms the observability of the thrown error.
I also think that treating unhandled exceptions as 500 responses is more similar to the actual server, and you write these request listeners from the server's respective.
*/ | ||
// xhrRequestController.errorWith(resolverResult.error) | ||
// Otherwise, error the request with the thrown data. | ||
xhrRequestController.errorWith(resolverResult.error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument for translating any other (non-error) data to request errors is a bit weaker. There should be a way to trigger request errors.
Now that I think about it, we do have that way! It's this:
request.respondWith(Response.error())
This translates to a request error across different clients. I think we need to remove non-error handling changes and treat non-errors as 500 responses anyway.
So the three cases become this:
- Throw a
Response
, it gets used as a mocked response. - Respond with
Response.error()
, it gets used as a request error. - Throw anything unhandled, it gets coerces to a 500 error response.
I think this makes the most sense.
This is what I eventually ended up with. I think it makes the most sense and makes the exception handling predictable. |
34c3dea
to
c46fd4d
Compare
This is great! |
Released: v0.28.3 🎉This has been released in v0.28.3! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Response
instance in a request listener, that instance will be used as a mocked response.