HTTPError code set to 'HTTPError' #1711#1739
Conversation
e320a5a to
ff56bc5
Compare
|
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 Probably best to follow the Node.js convention: https://nodejs.org/api/errors.html#nodejs-error-codes |
|
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 🙏 |
Correct. The logic has been moved to Line 1168 in bc9e48a Line 509 in bc9e48a Just omit setting |
You can submit a PR to the Note that current development for Got v12 is taking place in the |
ff56bc5 to
b40be7c
Compare
|
@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 |
| #### got.RequestError | ||
|
|
||
| When a request fails. Contains a `code` property with error class code, like `ECONNREFUSED`. All the errors below inherit this one. | ||
| When a request fails. Contains a `code` property with error class code, like `ECONNREFUSED`. If there is no specific code supplied `code` defaults to `ERR_GEN_REQ_ERROR` All the errors below inherit this one. |
There was a problem hiding this comment.
I think that ERR_GOT_REQUEST_ERROR would suit this a bit better. Wdyt?
There was a problem hiding this comment.
I like it. I didn't really love using 'general' error and this is more verbose.
| #### got.MaxRedirectsError | ||
|
|
||
| When the server redirects you more than ten times. Includes a `response` property. | ||
| When the server redirects you more than ten times. Includes a `response` property. Contains a `code` property with `ERR_MAX_REDIRECTS`. |
There was a problem hiding this comment.
What do you think about ERR_TOO_MANY_REDIRECTS?
|
The readme updates needs to be applied to the TypeScript types docs comments too. |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
|
Updates based on your feedback have been made! |
|
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? |
|
I can convert this PR to the |
|
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. |
|
@sindresorhus Let me know if this PR looks good. If so, I'll submit a PR for the |
|
I manually merged this into Got 12: f27e8d3 |
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