got icon indicating copy to clipboard operation
got copied to clipboard

Custom error in BeforeError hook

Open YaroslavRepeta opened this issue 5 years ago • 9 comments

Describe the bug

I was creating custom errors in beforeError hook. After upgrading to newer got version, it's not allowed by types (but still working if to cast types).

Actual behavior

I have to cast type.

Expected behavior

Return type for BeforeErrorHook is any Error.

Code to reproduce

Example of casting:

hooks: { beforeError: [error => new MyCustomError(error) as any]; }

Checklist

  • [x] I have read the documentation.
  • [x] I have tried my code with the latest version of Node.js and Got.

YaroslavRepeta avatar Jul 09 '20 17:07 YaroslavRepeta

What type does it require you to use?

sindresorhus avatar Jul 09 '20 18:07 sindresorhus

@sindresorhus RequestError - one what initially created by got (https://github.com/sindresorhus/got/blob/b9a855d3728d9219ddbc5d25b90d21147b982119/source/core/index.ts#L97)

So, technically it allows to add/change properties, but not change error itself.

YaroslavRepeta avatar Jul 09 '20 18:07 YaroslavRepeta

Your custom error must inherit from RequestError. Or you can just modify the error properties which would be more efficient.

szmarczak avatar Sep 15 '20 12:09 szmarczak

We should throw if the error we get from the beforeError hook is not an instance of RequestError.

szmarczak avatar Sep 15 '20 12:09 szmarczak

I would love to be able to return a custom object that gets thrown. I used to do something similar with axios. But with got, every time I try to add something to the error object, it throws the unmodified error.

Our main use case for this is to expose the most useful attributes of the error as we as to not log any of the request headers so we don't leak apikeys or other things we shouldn't log. Below is the beforeError handler I would like to use, but am not able to from what I can tell.

const beforeError = ({ name, stack, code, message, response }: RequestError) => {
  const { body, statusCode, retryCount } = response || {};

  return {
    name,
    message,
    stack,
    code,
    body,
    statusCode,
    retryCount
  };
};

coleabbeduto-NM avatar Oct 15 '20 21:10 coleabbeduto-NM

If anyone wants to see this happen in Got 12, now would be a time to submit a pull request, otherwise, we'll defer it to Got 13.

sindresorhus avatar Feb 26 '21 09:02 sindresorhus

Quick update on this. I was able to get it to allow custom error objects by extending the RequestError out of got. By not passing it the full error object in the super() call and setting my own properties on it with this.whatever, all of the custom properties show up and none of the things I don't want show up.

import got, { BeforeErrorHook, RequestError } from 'got';

export class CustomRequestError extends RequestError {
  public name = 'CustomRequestError';
  public timings: RequestError['timings'];
  public body: any;
  public statusCode: number;
  public retryCount: number;
  public url: string;

  constructor({ stack, code, message, timings, response, request }: RequestError) {
    super(message, {}, null);

    const { body, statusCode, retryCount } = response || {};

    this.stack = stack;
    this.code = code;
    this.timings = timings;
    this.body = body;
    this.statusCode = statusCode;
    this.retryCount = retryCount;
    this.url = request.requestUrl;
  }
}

const beforeError: BeforeErrorHook = (error) => new CustomRequestError(error);

coleabbeduto-NM avatar Feb 26 '21 16:02 coleabbeduto-NM

@coleabbeduto-NM

  constructor(error: RequestError) {
    super(error.message, error, error.request || error.options);

szmarczak avatar Feb 26 '21 18:02 szmarczak

I've got a slightly different angle on this case

I'm in the process of porting over an existing SDK to internally use got and am trying to intercept (among others) instances of HTTPError and trying to get an instance of a custom error class thrown instead, so that existing code that uses our SDK and does instanceof checking against certain classes won't break.

Am trying to do so via the beforeError hook, but it's not working: a RequestError gets thrown instead.

It seems that the destroy method (which is called after processing all of the beforeError hooks) does a check if the error instance is an instanceof RequestError and if not, creates a new RequestError instance and throws that instead

The reason I'm attempting to replace the RequestError instances with custom error instances is to make sure our SDK remains backwards compatible with the existing implementation that is not based on got (and also forwards compatible if we ever have the need to move away from got again)

The alternatively I've put in place now is to add a .catch() on every call to got in our SDK code and handle the conversion there, but I rather do that nice and cleanly, once, through hooks.

Unfortunately I do not have a good idea (yet) on how to allow the beforeError hook to return a non-RequestError instance, while making sure that plain JavaScript errors (like https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RangeError for example) do get converted to a RequestError instance (to prevent them from leaking), while at the same time allowing to handle those via the beforeError hooks.

Unless some config flags are introduced, which isn't very clean...

p-bakker avatar May 04 '21 09:05 p-bakker