fix: avoid Unexpected end of JSON input when response body is empty - #648
Conversation
|
The tests are failing because coverage fell below 100% After running 
I think in this particular case, we can add a // istanbul ignore next |
| try { | ||
| return response.json(); | ||
| } catch (e) { | ||
| return response.text(); |
There was a problem hiding this comment.
deleted suggestion, see below
There was a problem hiding this comment.
given that the await could easily get lost in a future refactoring, I think we should add a proper test for this case after all, instead of ignoring the case, to avoid future regression. Especially given that this is happening very sporadically, we wouldn't realize the regression for a longer time.
| // using .text(), but this should be investigated since if this were | ||
| // to occur in the GitHub API it really should not return an empty body. | ||
| try { | ||
| return response.json(); |
There was a problem hiding this comment.
You would likely need to use an await here in order for the try to actually catch anything since the error is presumably in the async phase of .json() rather than the sync execution.
Or do an "old-fashioned" promise usage like:
return response.json().catch(() => response.text())There was a problem hiding this comment.
We should cover this with a test after all instead of ignoring the line for coverage, this is too easy to miss.
|
I tried reproducing the "body used already for" error with native fetch but to no avail import { inspect } from "node:util";
const response = await fetch("https://httpbin.org/status/204");
const textResult = await response.json().catch(() => response.text());
console.log(inspect(textResult));I'm looking into it |
|
My guess is that it's a problem with The native I do not love it but we could add yet another catch and fallback to an empty string response.json().catch(() => response.text()).catch(() => '')We should update the comment to explain why |
|
I pushed my change. The suggested alternative is to only use - return response
- .json()
- // In the event that we get an empty response body we fallback to
- // using .text(), but this should be investigated since if this were
- // to occur in the GitHub API it really should not return an empty body.
- .catch(() => response.text())
- // `node-fetch` is throwing a "body used already for" error if `.text()` is run
- // after a failed .json(). To account for that we fallback to an empty string
- .catch(() => "");
+ const responseText = await response.text();
+
+ // Account for the possibility that GitHub returns an empty body
+ // or invalid JSON in an error response
+ if (!responseText.length) {
+ return "";
+ }
+
+ try {
+ return JSON.parse(responseText);
+ } catch (error) {
+ return responseText;
+ }Personally, I like the current version with the |
gr2m
left a comment
gr2m
left a comment
There was a problem hiding this comment.
Looks good to me, here is an alternative approach, but I like the current one better:
#648 (comment)
wolfy1339
changed the title
Unexpected end of JSON input when response body is empty
|
🎉 This PR is included in version 8.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* test: add test for empty response with json media type Follow-up to #648 * test: remove duplicate test
4 participants
Resolves #649
Before the change?
response.json()causing the error to be raised.After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!