Return 'null' on status 404 #57
Conversation
- Use const qualifiers - Initialize 'result' attribute with 'null', so the code matches the documentation - Use strict identity check for 'null' in test
|
|
|
Seems like a good change to take. @stephenmichaelf can you review? |
There was a problem hiding this comment.
@andrew8er Thanks for your contribution, we appreciate it! I added a few comments.
lib/RestClient.ts
Outdated
| rres.statusCode = statusCode; | ||
| const statusCode: number = res.message.statusCode; | ||
|
|
||
| const rres: IRestResponse<T> = { |
There was a problem hiding this comment.
Can we rename this to response or something more readable?
There was a problem hiding this comment.
I retained the original variable name, but yes, this should be renamed.
| @@ -38,75 +38,75 @@ describe('Rest Tests', function () { | |||
|
|
|||
| let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/get'); | |||
| assert(restRes.statusCode == 200, "statusCode should be 200"); | |||
| assert(restRes.result.url === 'https://httpbin.org/get'); | |||
| assert(restRes.result && restRes.result.url === 'https://httpbin.org/get'); | |||
There was a problem hiding this comment.
For all of these, I am not sure it adds value to have the restRes.result check before doing an equality check. It fails either way and since this is just a test I don't think we need it. Makes it harder to read too.
There was a problem hiding this comment.
Same comment for all places this has been added.
There was a problem hiding this comment.
I added these checks because I usually compile code with "strict" type checking enabled. If this option is disabled, then it would produces no error message, but in my eye, it is actually semantically wrong.
| assert(restRes.statusCode == 404, "statusCode should be 404"); | ||
| assert(restRes.result == null, "object should be null"); | ||
| assert(restRes.result === null, "object should be null"); |
There was a problem hiding this comment.
result object should be null?
There was a problem hiding this comment.
According to your documentation in https://github.com/Microsoft/typed-rest-client/blob/master/README.md a 404 should return null.
There was a problem hiding this comment.
Yep - in the REST layer since it's job is to return a restful object, a 404 should return a null. However the http layer will return body.
There was a problem hiding this comment.
I think I'm fine with these lines / changes
There was a problem hiding this comment.
I did a poor job of writing that comment. I meant that the string could be changed to be more expressive.
|
Actually I think the whole implementation of In the current version, exceptions from the JSON parser and response processor get swallowed and a |
|
@andrew8er - I'm fine not holding compat on "internal" underscore methods. A separate PR if you have a proposal for that. |
|
@andrew8er Could you rename that variable at the top? Then we can get this merged. The null check on the tests should be fine. As @bryanmacfarlane mentioned we can do a separate PR for refactoring _processResponse(). Thanks again! |
This PR changes
RestClient._processResult()to return a null result to match the behavior of the documentation. It previously returnedundefined. It also updates the signaure ofRestResponse<T>accordingly.