Use built-in fetch #1491
Use built-in fetch #1491
Conversation
kevcenteno
force-pushed
the
kevin/fetch
branch
2 times, most recently
from
93ef36a to
e8b57f9
Compare
kevcenteno
force-pushed
the
kevin/fetch
branch
from
e8b57f9 to
e0f5400
Compare
|
Should this be merged? |
|
Ah, we missed this. It should be merged |
oschwald
left a comment
oschwald
left a comment
There was a problem hiding this comment.
I am approving as I assume this is the same as it was for the first review, but I did have some questions/comments.
| modelClass?: any | ||
| ): Promise<T>; | ||
| private responseFor( | ||
| private async responseFor( |
There was a problem hiding this comment.
Should async be added to the method signature on line 62 too?
There was a problem hiding this comment.
It doesn't seem to be necessary.
src/webServiceClient.ts
Outdated
| const response = await fetch(url, options); | ||
|
|
||
| if (!response.ok) { | ||
| return Promise.reject(await this.handleError(response, url)); |
There was a problem hiding this comment.
I was a bit confused by this. I would have expected this to throw the exception directly (or even for handleError to do that.
I assume this is like this because you wanted to use the same catch for both the await fetch and the data = await response.json(). It would have been clearer to me to throw here and just rethrow instances of WebServiceClientError in the catch, but maybe the reasoning here is clear enough to most readers.
There was a problem hiding this comment.
I like how it separates handling "bad" server responses from client-side errors and aborts. I created a separate branch with the suggested change; let me know if it's what you had in mind: 65cd8fe
There was a problem hiding this comment.
Yeah, I guess that is what I had in mind, but I agree that it ends up being a bit messier than I would like. A comment or something might help people understand why it is like this.
Potentially, you could also move the response.ok check out of the try/catch too and pass the data into handleError, but that may be a bit more involved, especially for errors where the body isn't JSON.
src/webServiceClient.ts
Outdated
| } | ||
|
|
||
| if (response.status === 204) { | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
Is this different from just a bare return?
| return Promise.resolve(); | |
| return; |
src/webServiceClient.ts
Outdated
| const error = err as TypeError; | ||
| switch (error.name) { | ||
| case 'AbortError': | ||
| return Promise.reject({ |
|
I approved and merged it. |
No description provided.