Expose error codes #318
Expose error codes #318
Conversation
|
Hey @spruce, sorry this has sat so long. I'm getting back into OSS now and would love to help get this landed. To start, there's a merge conflict now. Mind taking a look at that? |
| @@ -879,6 +881,7 @@ describe('Unit | Mixin | ajax request', function() { | |||
| .catch(function(reason) { | |||
| expect(isTimeoutError(reason)).to.be.ok; | |||
| expect(reason.payload).to.be.null; | |||
| expect(reason.status).to.equal(-1); | |||
There was a problem hiding this comment.
Where did this -1 come from?
There was a problem hiding this comment.
| return this._createCorrectError(status, headers, payload, requestData); | ||
| }, | ||
|
|
||
| _createCorrectError(status, headers, payload, requestData) { |
There was a problem hiding this comment.
What's the reason for breaking this out into a separate method, rather than just adding the logic to the existing handleResponse?
There was a problem hiding this comment.
I actually came from a request you had: #275 (review)
Or did I misunderstand something there?
There was a problem hiding this comment.
That's fair 馃槄 That review was so long ago. I'm fine with merging as-is. I'm working on a pretty major refactor so might clean it up later if it's bugging me.
|
Will work on it. Would still like for this to get merged. I'm going to work on this now. |
There may be cases where callers want access to the status code that caused the error (e.g. logging/metrics, or terse user-facing error messages), so this allows them to easily get at it.
spruce
force-pushed
the
expose-error-codes
branch
from
fea88bd
to
b51e337
Compare
| return this._createCorrectError(status, headers, payload, requestData); | ||
| }, | ||
|
|
||
| _createCorrectError(status, headers, payload, requestData) { |
There was a problem hiding this comment.
That's fair 馃槄 That review was so long ago. I'm fine with merging as-is. I'm working on a pretty major refactor so might clean it up later if it's bugging me.
| @@ -879,6 +881,7 @@ describe('Unit | Mixin | ajax request', function() { | |||
| .catch(function(reason) { | |||
| expect(isTimeoutError(reason)).to.be.ok; | |||
| expect(reason.payload).to.be.null; | |||
| expect(reason.status).to.equal(-1); | |||
|
@spruce @alexlafroscia Thanks for getting this in! Could we get it into a patch release? I want my 502/503 codes 馃槃 |
|
Yeah, I think I should be able to cut a release, it doesn't seem like there's anything that's landed between then and now that would be breaking. Sorry this has lingered so long, I didn't realize the last major release was back in April! |
|
@alexlafroscia thank you! |
As written in #275 it would make sense to expose the actual http status codes outside of the default error classes.
Only thing I can see which blocked the PR was this: #275 (review)
So I tried to do everything of it. Only thing I couldn't achieve is to make it a switch statement. (I don't think it's possible, cause case expects only a value and not a expression and function calls are expressions. Right??? I'm really unsure but would like to know if I'm right or not)
Happy to work some more to get it merged 馃憖