Ky: Don't throw on `204 No Content` when parsing response

[psixdev]

Hello, I ran into a little problem. Not sure if this is a bug in the library or the expected behavior.
I have a little wrapper over the library:

const api = ky.create({
    prefixUrl: '...' // server api url
});

function request({path, ...params}) {
    return api(path, params)
        .json()
        .catch((err) => {
            if (err.response) {
                return err.response.json().then((err) => {
                    throw err;
                });
            } else {
                throw err;
            }
        });
}

It works great for get, post and patch requests, since they return json. However, it broke for delete request, which returned 204 No Content, with error SyntaxError: Unexpected end of JSON input.
Should the ky handle such situations and return null, for example? Or should I take care of this problem myself?
My current working solution:

function request({path, ...params}) {
    return api(path, params)
        .text()
        .then((text) => text && JSON.parse(text))
        .catch((err) => {
            if (err.response) {
                return err.response.json().then((err) => {
                    throw err;
                });
            } else {
                throw err;
            }
        });
}

[sholladay]

This is expected in the sense that it's exactly how fetch behaves and we have not indicated that Ky would do anything different here. It's also expected in the sense that an empty response '' is not valid JSON.

That said, I agree it's a bit of a bad UX. I am curious how other libraries handle this, if at all. I would be okay with skipping the JSON parsing when the response status is 204 No Content.

In my own apps, I went for returning a 200 OK with an empty object '{}' instead, as a workaround. Any valid JSON will work, including an empty double-quotes string '""'.

[psixdev]

I would not want to change the API and use 200 instead of 204 due to this library behavior. I think the problem should be solved on the client side, with the help of the library or not. I can’t promise, but I will try to find the time and see how other libraries do it or how it can be solved in ky.

[sholladay]

I guess we should return undefined. Feels a bit strange when the user asked for JSON, but so long as this only happens when the response code is explicitly 204, I suppose it's okay. Or we could return an empty object to play nice with code that is expecting JSON... 🤔

[psixdev]

In my opinion, the idea of returning undefined is better, since it allows us to distinguish between situations when the server returns an empty object and when it returns nothing. The main thing is the ability to handle the situation by the user. I can not imagine a situation where undefined can cause the user problems.
In either case, both solutions look better than the current behavior :)

[sindresorhus]

Let’s go with undefined and clearly document the behavior.

[szmarczak]

@sindresorhus I'd go with an empty object. People may check if (Reflect.has(response.body, 'error') etc.

[sholladay]

What about an empty string? It doesn't allow Reflect.has() to be used, but it's a valid JSON value while still being falsey like undefined. And an HTTP body is just text over the wire, so it's reasonable to expect an empty body to be represented as an empty string. Probably simplifies the TypeScript types, too, because .text() can remain Promise<string> instead of Promise<string | undefined>.

[sindresorhus]

@sholladay I like the empty string suggestion, especially because it keeps the types simple for the common-case.

[szmarczak]

If accept is application/json, then I'd go with an empty object. Otherwise an empty string.

[sholladay]

I made it return an empty string if .json() was used. So, more or less what @szmarczak said. Except I don't bother checking the header since it's the method itself that determines whether parsing is attempted, not the header.

By the way, I found out during my testing that .formData() has the same issue, it throws a parsing error if the body is empty. The other response types are unaffected (they simply return an empty blob, array buffer, etc). I'm going to punt on that for now. Maybe we'd want to return an empty FormData instance, though.

[szmarczak]

I made it return an empty string if .json() was used. So, more or less what @szmarczak said.

I said the opposite: unless .json() was used.

[sholladay]

Oh sorry, I read that wrong. Well an empty body is fine and doesn't cause any issues in most cases. For example, if you don't use any of the body methods then you're fine. Or if you use .text(), then you'll get an empty string. That's always been the case. I'm just going to fix the parsing error in .json() and replace it with an empty string in that specific case. An empty object was my first thought, but since an empty string is still valid JSON, I think it makes more sense to do that, as an empty string is falsey.

[szmarczak]

Alright then. Let's do it that way :)