Return undefined instead of empty string from .json() on empty responses#828
Return undefined instead of empty string from .json() on empty responses#828sindresorhus merged 4 commits intomainfrom
undefined instead of empty string from .json() on empty responses#828Conversation
| // ergonomic "no content" handling. | ||
| if (response.status === 204) { | ||
| return ''; | ||
| return undefined; |
There was a problem hiding this comment.
I'm aware it could be just return, but maybe having undefined there explicitly makes it slightly more readable.
sholladay
left a comment
There was a problem hiding this comment.
Code LGTM but my opinion remains that we should return valid JSON if the method does not throw.
What exactly do you propose then?
|
|
My thinking was that an empty body is equivalent to an empty string, because HTTP is just text over the wire. And you can get an empty string back from the JSON parser. JSON.parse('""') === '';
// => trueI take it back, though. I thought that My other main reason for preferring an empty string over Let's do it. 👍 |
| ``` | ||
| */ | ||
| json: <J = T>() => Promise<J>; | ||
| json: <J = T>() => Promise<J | undefined>; |
There was a problem hiding this comment.
Not sure you'll see this since it's merged, but widening to Promise<J | undefined> means that we have to guard against it at call sites which I wouldn't expect when explicitly passing in expected return type?
const response = http.get(`projects/${projectName}/build-number`).json<BuildNumberResponse>();
if (response !== undefined) { ... }
There was a problem hiding this comment.
@sindresorhus I've seen about 10 comments on this so far. Seems to be the most common issue people run into with 2.0.
Fixes #732