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

Custom errors thrown in afterResponse are suppressed #1352

Closed
2 tasks done
simontabor opened this issue Jul 9, 2020 · 7 comments
Closed
2 tasks done

Custom errors thrown in afterResponse are suppressed #1352

simontabor opened this issue Jul 9, 2020 · 7 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@simontabor
Copy link

Describe the bug

If my afterResponse hook throws a custom error, I'm not expecting it to be rewritten into a Got Request Error

Actual behavior

Error is rewritten as a RequestError, with no reference to the original error

RequestError: Bad request signature

Expected behavior

Original error is thrown

HmacError: Bad request signature

Code to reproduce

const got = require('got');
const assert = require('assert');

class HmacError extends Error {}

got({
  url: 'https://not-here.com',
  hooks: {
    afterResponse: [
      () => {
        throw new HmacError('Bad request signature');
      },
    ],
  },
}).catch(err => {
  assert.equal(err.name, 'HmacError');
});

This will throw -

(node:73559) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: 'RequestError' == 'HmacError'

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@simontabor simontabor changed the title Custom errors thrown in afterResponse are edited Custom errors thrown in afterResponse are suppressed Jul 9, 2020
@szmarczak
Copy link
Collaborator

This behavior is intended as the Got errors provide useful properties for debugging. Instead of overwriting the error, we could write into it.

@szmarczak szmarczak added enhancement This change will extend Got features ✭ help wanted ✭ labels Jul 9, 2020
@simontabor
Copy link
Author

@szmarczak that makes sense... I'd rather we treat the errors as immutable (otherwise you can end up in a right old mess with Typescript!)

How about we keep a reference to the originalError within the RequestError?

@simontabor
Copy link
Author

Just also noticed from the error that only the timings are attached to the error - how can I access the underlying request or response when my afterResponse handler throws an error? Might be worth attaching request and response wherever possible too (potentially non-enumerable), I'd like to see what failed request was rather than just the timings

@szmarczak
Copy link
Collaborator

szmarczak commented Jul 10, 2020

Actually if it's missing request and response properties at this point then it's a bug... It should have that already.

@simontabor
Copy link
Author

You're right, sorry about the last comment, didn't even check - it's there but non-enumerable already so didn't show up in my console.log

@szmarczak
Copy link
Collaborator

It's non-enumerable so if you use Sentry or any other error-catching API you won't get bloated data. Although I think we could fix that. See #1067

@szmarczak
Copy link
Collaborator

Closing in favor of #1353

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

2 participants