Migrating the auth module to use the new HTTP Client #291
Conversation
| this.response = new DefaultHttpResponse(resp); | ||
| constructor(public readonly response: HttpResponse) { | ||
| super(`Server responded with status ${response.status}.`); | ||
| Object.setPrototypeOf(this, HttpError.prototype); |
There was a problem hiding this comment.
What does this actually do? You are setting prototype of HttpError to prototype of HttpError?
There was a problem hiding this comment.
This is needed for the instanceof checks to work correctly: microsoft/TypeScript#13965
There was a problem hiding this comment.
Can you add a comment on this. There is no way I would have guessed this.
| }; | ||
| } | ||
|
|
||
| export function errorFrom(data: any, status: number = 500): HttpError { |
There was a problem hiding this comment.
If I recall, the default error http status for majority of errors above is 400. It is 500 here.
There was a problem hiding this comment.
I don't think it matters that much. The old test cases did not use any status codes, which implies that the auth module is agnostic to the actual HTTP status codes.
With this change we now have test cases that return both 500 and 400 errors. WDYT?
There was a problem hiding this comment.
In the past there was no indication of http status code during test simulation. With this change, it is implied all simulated status codes are 500s. It is true Auth doesn't care in this case, but other services may. Note this could also change in the future. 500 could be handled differently.
Anyway, this is probably fine for now.
2 participants
#280 introduced a new set of HTTP client abstractions. This PR migrates the auth module to use them.The resulting code is shorter (about 150 lines less) and easier to test.