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

Improve error handling #18

Open
ChuckJonas opened this issue Jun 15, 2023 · 3 comments
Open

Improve error handling #18

ChuckJonas opened this issue Jun 15, 2023 · 3 comments

Comments

@ChuckJonas
Copy link
Contributor

Right now the error handling only exposes the response body via the standard Error class.

  1. This makes it hard to know if the exception was a result of this library or something else
  2. You do not have access other response properties (most importantly statusCode

I'd probably copy axios and do something like this:

class HttpResponseError extends Error {
  httpResponse: Response;

  constructor(message: string, httpResponse: Response) {
    super(message);

    // Set the prototype explicitly to allow instanceof checks
    Object.setPrototypeOf(this, HttpResponseError.prototype);

    this.name = 'HttpResponseError';
    this.httpResponse = httpResponse;
  }
}

Or maybe even just have properties forstatus, body, etc if it's already been parsed/consumed.

Then you can do something like this in your catch blocks:

}catch(e){
  if(e instanceof HttpResponseError){
     if(e.statusCode === 429){
        // retry request with backoff
        return retry();
     }
  }
  throw e;
}
@lino-levan
Copy link
Collaborator

I think it would be nice if we retry 429 errors by default... definitely a footgun that we do not.

@ChuckJonas
Copy link
Contributor Author

think it would be nice if we retry 429 errors by default... definitely a footgun that we do not.

Typically, I'd leave that up to the consumer. Otherwise, it makes it harder to compose into circuit breakers or more general async retry frameworks.

If that feature was implemented, I would definitely want a way to configure it (number of retries, expo backoff, alternate server, etc) and would default it to off.

IMO, there are too many good libraries out there that support this already, this one should only be concerned with exposing the information needed to hook into those (raw request/response info)

@lino-levan
Copy link
Collaborator

Being configurable makes sense to me. My thinking is that having it on by default is a good, and users with more advanced usecases can just disable it and handle it themselves.

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

2 participants