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

HTTPError code set to 'HTTPError' #1711 #1739

Merged
merged 5 commits into from Jun 17, 2021

Conversation

Drewfergusson
Copy link

@Drewfergusson Drewfergusson commented Jun 2, 2021

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)

@Drewfergusson Drewfergusson force-pushed the andrew-HTTPError-code branch 2 times, most recently from e320a5a to ff56bc5 Compare Jun 2, 2021
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jun 2, 2021

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 Drewfergusson mentioned this pull request Jun 3, 2021
1 task
@Drewfergusson
Copy link
Author

@Drewfergusson Drewfergusson commented Jun 13, 2021

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
Copy link
Collaborator

@szmarczak szmarczak commented Jun 13, 2021

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.

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

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

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

@szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Jun 13, 2021

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 Drewfergusson changed the base branch from main to v11 Jun 14, 2021
@Drewfergusson
Copy link
Author

@Drewfergusson Drewfergusson commented Jun 14, 2021

@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.

readme.md Outdated
@@ -1705,43 +1705,44 @@ Additionaly, the errors may have `request` (Got Stream) and `response` (Got Resp

#### 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.
Copy link
Collaborator

@szmarczak szmarczak Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Drewfergusson Drewfergusson Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

readme.md Outdated

#### 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`.
Copy link
Collaborator

@szmarczak szmarczak Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about ERR_TOO_MANY_REDIRECTS?

@szmarczak szmarczak requested a review from sindresorhus Jun 14, 2021
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jun 15, 2021

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

@Drewfergusson
Copy link
Author

@Drewfergusson Drewfergusson commented Jun 15, 2021

Updates based on your feedback have been made!

@szmarczak szmarczak requested a review from sindresorhus Jun 16, 2021
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jun 16, 2021

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
Copy link
Collaborator

@szmarczak szmarczak commented Jun 16, 2021

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

@Drewfergusson
Copy link
Author

@Drewfergusson Drewfergusson commented Jun 17, 2021

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
Copy link
Collaborator

@szmarczak szmarczak commented Jun 17, 2021

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

@sindresorhus sindresorhus merged commit 0e167b8 into sindresorhus:v11 Jun 17, 2021
szmarczak added a commit that referenced this issue Jul 1, 2021
@szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Jul 1, 2021

I manually merged this into Got 12: f27e8d3

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

Successfully merging this pull request may close these issues.

None yet

3 participants