HTTPError code set to 'HTTPError' #1711

[Drewfergusson]

This is my understanding of the intention of #1711. This sets the code property on the HTTPError object to 'HTTPError' instead of leaving it as undefined.

Checklist

• I have read the documentation.
• I have included a pull request description of my changes.
• I have included some tests (updated failing test to pass)

[sindresorhus]

I think it should be done on all errors at once so we can ensure we do it consistently.

And we should use a better code than HTTPError. It doesn't need to include the word Error either.

Probably best to follow the Node.js convention: https://nodejs.org/api/errors.html#nodejs-error-codes

[Drewfergusson]

Hey, I was working on a solution for this but now I'm a little confused.

I made this PR against the main branch because I saw others doing that in the closed PR's. I just realized how different the v11.8.2 branch is from the main branch. The main branch has errors broken out into a separate file and is missing the UnsupportedProtocolError class the v11.8.2 has.

Am I missing something here? I searched for how to contribute but if you could point me to any docs that would be helpful 🙏

[szmarczak]

The main branch [...] is missing the UnsupportedProtocolError class the v11.8.2 has.

Correct. The logic has been moved to options.ts where all normalization happens. The error is later replaced by RequestError.

got/source/core/options.ts

Line 1168 in bc9e48a

throw new Error(`Unsupported protocol: ${url.protocol}`);

got/source/core/index.ts

Line 509 in bc9e48a

error = new RequestError(error.message, error, this);

Just omit setting .code for this one. Errors originating from options.ts need a separate discussion.

[szmarczak]

I searched for how to contribute but if you could point me to any docs that would be helpful 🙏

You can submit a PR to the v11 branch, so another v11 version will be released.

Note that current development for Got v12 is taking place in the main branch.

[Drewfergusson]

@szmarczak let me know any feedback on this latest.

I realized that some of the error objects have a 'code' property passed in (but not always) and didn't want to overwrite that if it contained a more specific error code for users. So I changed the code to only add the error codes if one is not passed in. One change from our discussion was adding a default code for the parent RequestError object of ERR_GEN_REQ_ERROR for situations where no code is passed in.

[szmarczak]

I think that ERR_GOT_REQUEST_ERROR would suit this a bit better. Wdyt?

[Drewfergusson]

I like it. I didn't really love using 'general' error and this is more verbose.

[szmarczak]

What do you think about ERR_TOO_MANY_REDIRECTS?

[sindresorhus]

The readme updates needs to be applied to the TypeScript types docs comments too.

[Drewfergusson]

Updates based on your feedback have been made!

[sindresorhus]

I'm confused, why is this pull request targeting the v11 branch? The common practice is to always first land a change on the main branch and then a feature branch (v11) if needed. What's the plan here? Do you plan to submit another pull request with changes to the main branch too?

[szmarczak]

I can convert this PR to the main branch, this should be easy, but I don't mind releasing another v11 version first.

[Drewfergusson]

Yeah, with some adaptation this can be added to the v12 branch. Let me know if you want me to work on a separate PR for the v12 branch.

[szmarczak]

@sindresorhus Let me know if this PR looks good. If so, I'll submit a PR for the main branch.

[szmarczak]

I manually merged this into Got 12: f27e8d3