Skip to content
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

Uniformize Error Types #226

Closed
lpellegr opened this issue Jan 25, 2020 · 7 comments
Closed

Uniformize Error Types #226

lpellegr opened this issue Jan 25, 2020 · 7 comments

Comments

@lpellegr
Copy link

While using ky I noticed that errors thrown are not uniformized. Sometimes a custom ky error type is used and sometimes a raw TypeError is thrown.

Let's say you have a web app with a function whose purpose is to display error messages based on error types, then managing the different ky options is just a mess. For instance, when the URL considered is not reachable a TypeError is thrown but a call with failing HTTP calls uses a custom HTTPError. In the former case, when a TypeError is thrown, there no easy means to know in the browser that the error is a ky error: the value is an empty object.

I would suggest using custom error types for all errors with a root one that extends Error.

@sholladay
Copy link
Collaborator

I'm not very convinced. The purpose for the different error types is precisely so that you can differentiate them. If we are using them inconsistently then that is worth fixing. But I don't think we are going to make a custom error class for type errors, for example. We use RangeError, TypeError, and ky.HTTPError where appropriate for the specific error condition. They all extend from Error already. We use generic Error for certain validation errors, which is probably the only thing I would consider changing here. We could have a ky.ValidationError or something. But I'm skeptical of how much value such a class would really provide. They are unrecoverable errors so there is not much you can do with them. Perhaps we could improve the messages, though? In any case, you can figure out if the error came from Ky by inspecting the stack trace, which is available at error.stack.

@lpellegr
Copy link
Author

lpellegr commented Jan 25, 2020

@sholladay Thanks for your feedback.

Let's say you want to display a custom message "Cannot reach our servers. Are you connected to the Internet?" on your web app when a call fails due to an unreachable URL. In that case, ky throws a TypeError (when the timeout parameter is set to 0), as for ky configuration errors. How do you distinguish between both? should I really inspect error.stack?

@sholladay
Copy link
Collaborator

sholladay commented Jan 25, 2020

What is the actual error message you are seeing? Do you have a reproducible example? It sounds like you are talking about an error that comes from fetch, not from Ky itself. Assuming I am correct about that, it's not something we directly control. How do you propose we detect and normalize it?

The way I do a custom message for network errors in my apps is to check !error.response, as it indicates that something failed before a response was received, such as a network error (including timeouts) or validation error. I then filter out validation errors as best I can, which is the part that is a little messy, and could be improved a bit. But again, there are validations that fetch performs and there are validations that Ky performs. I'm not sure how feasible it is to normalize all of them. But we could try.

It would be nice to have an example of such error handling in our documentation, including the !error.response trick, if we don't already have that.

@szmarczak
Copy link
Collaborator

I think this problem could be solved via beforeError hook like in Got.

@s-pic

This comment has been minimized.

@sholladay
Copy link
Collaborator

sholladay commented Mar 9, 2020

@GeoMC you didn't show all of your code and I don't know how api works, but my guess is one of the following:

  1. Your api implementation might be expecting an options object, not a body object. So Ky might be receiving an option named hello instead of body.
  2. fetchOptions.body is probably a string, not an object, because Ky has to serialize the body before passing it to fetch(). Make sure your assertion is taking that into account.

Your problem really has nothing to do with this issue thread you commented on here. In the future, please ask support questions on StackOverflow or open a new issue.

@sholladay
Copy link
Collaborator

Closing due to lack of activity and specific action items. I agree that error handling for fetch() is less than ideal, and this inherently impacts Ky as a result. I'm not sure what exactly to do about it, though. If anyone has specific suggestions please comment here or send a PR and we can consider reopening this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants