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

Ensure HTTPError always has a defined message #249

Merged
merged 5 commits into from
Apr 17, 2020
Merged

Ensure HTTPError always has a defined message #249

merged 5 commits into from
Apr 17, 2020

Conversation

kumar303
Copy link
Contributor

@kumar303 kumar303 commented Apr 8, 2020

Fixes #248

This ensures HTTPError will always have a defined message in various situations.

@yunyu
Copy link

yunyu commented Apr 8, 2020

This would also help with Sentry error reporting.

Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

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

I like the implementation but for the tests, I would prefer to use more realistic objects and patterns that align better with the other existing tests. For example, use an actual test server that responds without status text. Or instead of http.ServerResponse, return a WHATWG Response inside of a hook.

index.js Outdated Show resolved Hide resolved
@kumar303
Copy link
Contributor Author

kumar303 commented Apr 9, 2020

Thanks for the quick review!

I like the implementation but for the tests, I would prefer to use more realistic objects and patterns that align better with the other existing tests. For example, use an actual test server that responds without status text.

Yeah, I struggled with this a bit. I couldn't see an easy way to make a response with an undefined statusText using the test server (it doesn't provide low enough access to the response).

Or instead of http.ServerResponse, return a WHATWG Response inside of a hook.

Ah, yes, a WHATWG Response would be more realistic. I'll try that.

@kumar303
Copy link
Contributor Author

@sholladay I switched to a proper fetch Response. Let me know how it looks.

index.js Outdated Show resolved Hide resolved
@kumar303
Copy link
Contributor Author

@sholladay this supports status codes of 0 now.

@kumar303 kumar303 requested a review from sholladay April 15, 2020 22:03
// Assign these values because the Response() constructor doesn't
// support setting them to undefined.
response.status = status;
response.statusText = statusText;
Copy link
Collaborator

@sholladay sholladay Apr 15, 2020

Choose a reason for hiding this comment

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

These can be set in the init argument for the Response constructor. Should be eas easy as...

const error = new ky.HTTPError(new Response(null, { status: 200, statusText: 'Foo bar' }));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work because these values need to be set to undefined and from looking at the node-fetch source, they are using old school truthy checks to tell if these values are set (thus, they can never be set to undefined).

I realize it makes the code look a bit odd -- that's why I added a comment explaining it.

@kumar303 kumar303 requested a review from sholladay April 16, 2020 14:32
@kumar303
Copy link
Contributor Author

@sholladay let me know if my response makes sense. Thanks.

Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

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

LGTM.

Might be nice, though, to add parentheses around the OR condition (... || ...) to make it clear that it has higher precedence than the ternary.

@sindresorhus sindresorhus merged commit d5e1233 into sindresorhus:master Apr 17, 2020
@kumar303 kumar303 deleted the fix-http-error branch April 17, 2020 21:27
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.

An undefined HTTPError message breaks New Relic
4 participants