json apis #9
json apis #9
Conversation
|
This will offer pretty equivalent behavior to the calls in cache and artifacts that rely on all the json request and process request |
| @@ -191,5 +198,45 @@ describe('basics', () => { | |||
| expect(res.message.statusCode).toBe(404); | |||
| let body: string = await res.readBody(); | |||
| done(); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Is it worth adding a test for the Date parsing?
There was a problem hiding this comment.
I'll create an issue for that. I'll get this in to unblock the other work.
|
merging on CR from Josh to unblock other work. @ericsciple and @dakale if you have any other feedback, we can follow up with issues. |
| } | ||
|
|
||
| // note that 3xx redirects are handled by the http layer. | ||
| if (statusCode > 299) { |
There was a problem hiding this comment.
nice, this takes the error handling burden off each consumer
| // Invalid resource (contents not json); leaving result obj null | ||
| } | ||
|
|
||
| // note that 3xx redirects are handled by the http layer. |
There was a problem hiding this comment.
3xx redirects are handled by the http layer
this comment is not true depending on redirect setting IRequestOptions.maxRedirects
There was a problem hiding this comment.
Also 304 is not an error.
Would it make more sense to change the below line to if (statusCode > 399) { since only 400 and above are errors?
There was a problem hiding this comment.
Nevermind, the way you have it makes more sense. This function is called when the user is expecting a payload (so generally 2xx range).
Since you added response to error, client can catch and handle appropriately.
| // attach statusCode and body obj (if available) to the error object | ||
| err['statusCode'] = statusCode; | ||
| if (response.result) { | ||
| err['result'] = response.result; |
There was a problem hiding this comment.
clever returning these properties on the error
since the body has already been ready, would it make sense to include err['body'] = contents
| // if exception/error in body, attempt to get better error | ||
| if (obj && obj.message) { | ||
| msg = obj.message; | ||
| } else if (contents && contents.length > 0) { |
There was a problem hiding this comment.
nit: technically contents.length > 0 is unnecessary since empty string is falsy
|
|
||
| // if exception/error in body, attempt to get better error | ||
| if (obj && obj.message) { | ||
| msg = obj.message; |
There was a problem hiding this comment.
nit: For error cases, should the http status code always be included in the message too? Seems likely obj.message or contents alone sometimes not helpful or as useful for debugging
| contents = await res.readBody(); | ||
| if (contents && contents.length > 0) { | ||
| if (options && options.deserializeDates) { | ||
| obj = JSON.parse(contents, HttpClient.dateTimeDeserializer); |
There was a problem hiding this comment.
dateTimeDeserializer
Feels gross since it's not based on the metadata of the type. I guess that's the way it has to be in js. And opt-in so whatevs :)
For my own benefit, curious about the specific use case?
There was a problem hiding this comment.
yep. this carried over from the typed rest client
| return this._processResponse<T>(res, this.requestOptions); | ||
| } | ||
|
|
||
| public async postJson<T>(requestUrl: string, obj:T, additionalHeaders?: ifm.IHeaders): Promise<ITypedResponse<T>> { |
There was a problem hiding this comment.
consider adding comments, also consider >= 300 rejects promise
| return this._processResponse<T>(res, this.requestOptions); | ||
| } | ||
|
|
||
| public async postJson<T>(requestUrl: string, obj:T, additionalHeaders?: ifm.IHeaders): Promise<ITypedResponse<T>> { |
There was a problem hiding this comment.
This doesnt seem to work at least for actions/cache types like ReserveCache[Request / Response] since in input type T = ReserveCacheRequest DNE the return type T = ReserveCacheResponse
Need to think on it more, or maybe someone else has ideas here, but it seems to me that the input type does not need to be T and the type parameter should only refer to the return type that we parse into. You dont gain much by specifying the input type since... its just the type you pass in. The typed-rest-client takes any here instead of T
There was a problem hiding this comment.
Changing the input type T => any shouldnt be breaking because any T is assignable to any, if we go that route
create, post, put and patch a json object.
These are convenience wrappers.
Not they do not imply REST (and the rest goals) although they can be used to access rest apis of a service (that work with json).