Throws more useful error when fetch fails #2921
Conversation
There was a problem hiding this comment.
If the goal is to have human readable errors suitable for presentation in a UI, then starting the string with an http error code is not great.
If we are going to have something presentable like "load failed" that string should come first.
-
Apps should no longer rely on string parsing but use the cause
-
If we add a cause mighn't we as well create a LoadError class so that callers can reliably detect that this is from loaders and we can type the cause.
-
Based on experience apps will not use the cause, it would be there for frameworks like deck.gl so make sure it meets your needs.
-
to set expectations, since this affects apps, I would want this to be a 4.2 feature with some what's new documentation, not slip it in as a patch release. If this is a blocker for deck.gl maybe we can do this in two parts - add the 'cause' for deck.gl as a patch in 4.1 and sweat the message changes in 4.2
| const message = await getResponseError(response); | ||
| throw new Error(message); | ||
| const error = await getResponseError(response); | ||
| throw error; |
There was a problem hiding this comment.
I guess creating the error inside makes the error stack trace point to a different place. just trying to consider the debug experience, but this is probably ok.
There was a problem hiding this comment.
The stack trace will be getResponseError ... checkResponse ... <caller of checkResponse> ... so basically one extra layer on top of whatever it was before. If it matters to you then we can move the content of getResponseError into this function since this function doesn't do anything else.
| text += ` ${await response.text()}`; | ||
| cause.response = await response.json(); | ||
| } else { | ||
| cause.response = await response.text(); | ||
| } |
There was a problem hiding this comment.
json can be very messy and if this is e.g. xml it can get crazy. This is part of why including this verbatim is so problematic.
|
I am fine with putting the error code after the message, i.e. "Failed to fetch resources (500)"
What app is this affecting exactly? Is any app relying on a trimmed URL in the message? I have a hard time believing it because the following message is 60 characters: |
| let message = `Failed to fetch resource (${response.status}): ${response.url}`; | ||
| message = message.length > 65 ? `${message.slice(0, 65)}...` : message; | ||
|
|
||
| const cause: { |
There was a problem hiding this comment.
IMHO, cause is really intended to be used for linking to another exception. It is used when rethrowing exceptions to link to the original Error.
While the MDN docs do state that cause does not have to be an Error instance, to me this is similar to saying that a caught exception is not guaranteed to be an Error instance. Technically it can happen but it is unlikely and very annoying when it does.
One problem with supplying cause as an object is that there is no way to check what type it is, in contrast to exporting an Error subclass where the app can use instanceof to safely access any additional members.
| if (contentType?.includes('application/json')) { | ||
| text += ` ${await response.text()}`; | ||
| cause.response = await response.json(); |
There was a problem hiding this comment.
If we do have a response field on the error shouldn't it be the actual Response object?
Same response, before:
After: