fix(parsestatus): add response data to error #510
Conversation
ghost
marked this pull request as ready for review
ghost
mentioned this pull request
There was a problem hiding this comment.
We'll need to change the Typescript type of FetchError.details - it doesn't complain because the response.json() function returns an any type, but accessing it later and expecting arbitrary JSON should complain.
Other than that the change looks good to me! One minor optional comment (feel free to ignore).
I suppose we'll need to make sure this is flagged as a BREAKING CHANGE when merging too, though it's a silly one. Perhaps we could call this a "fix" since it was never specified in the docs and accessing the body of the response never worked?
I checked all the places I thought we might possibly be using this and only found one (unpublished) which shows we need to add an error type for Not Found (catching 404 errors)
Yeah a fix is fine with me. It all depends on what the intended api was I guess. So you'd prefer fix? |
|
I think I've addressed all the comments with the above commits, and I've added tests where necessary. Let me know how you feel about the current state of the PR, whether you'd prefer a fix or a breaking change and then this should be good to go! |
ghost
force-pushed
the
change-parse-status
branch
3 times, most recently
from
1220f1c to
7b679d2
Compare
Yeah, it was basically broken before so let's say this is a fix. |
ghost
force-pushed
the
change-parse-status
branch
from
16a8c53 to
574932b
Compare
No problem, I think it's good to think these kinds of changes over. I think I've addressed all the comments and I've removed the breaking change label. If you're ok with it let me know, I propose before merge we squash to the first commit message. |
The error returned for failed fetches no longer contains the response under error.details. Instead error.details now contains the parsed response body, if it could be parsed.
ghost
force-pushed
the
change-parse-status
branch
from
7d6d754 to
7dc6978
Compare
|
All right, done and done. Thanks for the help @amcgee 👍! |
## [2.1.2](v2.1.1...v2.1.2) (2020-04-22) ### Bug Fixes * **parsestatus:** add response data to error ([#510](#510)) ([5c124db](5c124db))
|
🎉 This PR is included in version 2.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
As discussed on app-platform. The body of the response that was attached to the error was already parsed, so once it reaches the app, it can't be parsed a second time and the data was lost.
This attaches the parsed data to the error under error.details (it if can be parsed). So error.details will now no longer be the response object.
p.s.: I used a try catch block as I found the synchronous notation easier to read in combination with async/await, but that's a bit subjective, so let me know if you'd like me to use the promise api.