-
Notifications
You must be signed in to change notification settings - Fork 131
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
Implement better messages for request errors #1274
Conversation
While debugging some issues, we noticed that our network-related error messages could do with more verbosity. This is what this commit does. It also adds the HTTP status code in the message when this is the source, of the network error. Today it is only present as a `status` property.
949beee
to
982f912
Compare
src/errors/request_error.ts
Outdated
|
||
switch (type) { | ||
case "TIMEOUT": | ||
this.message = "The request timeouted"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better English would be
this.message = "The request timeouted"; | |
this.message = "The request timed out"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does sound better :p!
Thanks!
3447741
to
c161a5b
Compare
@@ -50,7 +50,8 @@ describe("errors - NetworkError", () => { | |||
expect(networkError.code).toBe("PIPELINE_LOAD_ERROR"); | |||
expect(networkError.fatal).toBe(true); | |||
expect(networkError.message) | |||
.toBe("NetworkError (PIPELINE_LOAD_ERROR) ERROR_HTTP_CODE"); | |||
.toBe("NetworkError (PIPELINE_LOAD_ERROR) An HTTP status code " + | |||
"indicating failure was received: 13"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing issue in the test: 13 isn't a normal HTTP status code. I would expect, for example, 403.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
c161a5b
to
7daec74
Compare
7daec74
to
f8862f0
Compare
While debugging some issues, we noticed that our network-related error messages could do with more verbosity. This is what this PR does.
It also adds the HTTP status code in the message when this is the source, of the network error. Today it is only present as a
status
property.