Improve error handling of responses - #821
Conversation
6dd6a2b to
3727d7d
Compare
3727d7d to
60970d0
Compare
|
@chvp I'll leave this as a draft until I have a matching PR in the web client. I imagine some details might still change while working on the web side, but this should generally be good for a first review |
|
|
||
| // If the body isn't json, we throw an error with the body parsed as plain text | ||
| const contentType = response.headers.get("Content-Type"); | ||
| if (contentType === null || !contentType?.startsWith("application/json")) { |
There was a problem hiding this comment.
This is a naive way of checking this. Maybe we should properly parse this header and check the value?
| export class UnknownError extends Error { | ||
| details?: unknown; | ||
|
|
||
| constructor(message: unknown, details?: unknown) { | ||
| super(`${message}`); | ||
| this.name = "UnknownError"; | ||
| this.details = details; | ||
| } | ||
| } | ||
|
|
||
| export class UnexpectedError extends Error { |
There was a problem hiding this comment.
I wanted to distinguish between:
- The rails API returns an error that we didn't explicitly send (fe a 500 internal server error)
- We get an error that we truly don't know what to do with (fe. getting a non-json body)
I don't think this is the best potential naming, but it's what I could come up with
| const reason: Record<string, string[]> = {}; | ||
| if (error instanceof Error) { | ||
| reason[error.constructor.name] = [error.message]; | ||
| throw error; |
There was a problem hiding this comment.
In the future, we might want to check the content of these errors and return custom types that wrap these so that it's easier to handle them in the client (for example with an OfflineError)
There was a problem hiding this comment.
Yeah I did that at work in the recent past, it's quite nice to do this (since otherwise it's just a TypeError in most browsers).
| errors: Array<AnyError>; | ||
| }; | ||
|
|
||
| export type AnyError = |
There was a problem hiding this comment.
| export type AnyError = | |
| export type AnyErrorMessage = |
? Since this isn't a subclass of Error this feels a bit nicer to me.
|
|
||
| // If the body isn't json, we throw an error with the body parsed as plain text | ||
| const contentType = response.headers.get("Content-Type"); | ||
| if (contentType === null || !contentType?.startsWith("application/json")) { |
2 participants
This PR improves the way we handle errors in
resolve. This should help clients to distinguish between the different errors and handle each in a specific way.resolveis now a subclass ofError, following the advice on MDNI didn't really add tests now, but I have an idea for more extensive tests for the api client (using VCR like recordings) - I'd rather do that then create a bunch of tests that write out a reponses by hand