Why is data an empty object for a 204 response?

[mhuttner]

I ran into this issue for an api call that may return 204.

If i check the returned data i expect it to be null

const { data } = await client.GET("/api/route/204");
if (data === null) {
 // do something
}
const { field} = data;

But this library explicitly sets the data to be an empty object?!

openapi-typescript/packages/openapi-fetch/src/index.js

Lines 156 to 160 in 2a4b067

	// handle empty content
	// note: we return `{}` because we want user truthy checks for `.data` or `.error` to succeed
	if (response.status === 204 || response.headers.get("Content-Length") === "0") {
	return response.ok ? { data: {}, response } : { error: {}, response };
	}

Is there a reason for this?
This also breaks the type system, because it expects data to be the full object, null or undefined but not {}.

[drwpow]

Is there a reason for this?

As stated in the comment, it’s to cover the really common scenario where people type

if (data) {
  // …
} else {
  throw new Error()
}

If we make that null, then this code won’t throw an error on any 2XX code, but it will for 204, which breaks the pattern, and will get more bug reports than the current implementation (probably).

Also, to someone consuming this library, they don’t deal directly with the status code, and would be forgiven for writing this code without first consulting that an API may respond with 204 (which is successful).

To your point, this may not be technically correct in all scenarios. But returning data: null in JavaScript would cause more footguns, and more unexpected errors, than {} would. But to your point, this does cause a mismatch in the type, yes.

How would you propose we fix this? Would it have to involve saying “never ever write if (data) in your code?” in the docs, e.g.? (I’m actually not being rhetorical or dismissing the question; I am legitimately open to hearing arguments for encouraging people to write code a certain way! Ideally it’s a balance between the code working as-expected, but also encouraging users to go about using it a certain way within reason)

[mhuttner]

Hey,
thanks for the quick response. I would argue throwing in this case is the desired behaviour.
If the user checks the data exists, it is also assumed that it matches the type of the type system. The code in your example will then use data in some way and might then throw unexpectedly anyway. This is what happened to me. I can give a more discrete example:

const { data: user } = await api.GET("/user/me"); // returns the user, or 204
if (!user) {
  setState("unauthenicated")
  return;
}
const  { keys } = user;
if (keys.length === 0) {  // This throws
 setState("no_keys")
}

I have to do this now:

const { data: user, response } = await api.GET("/user/me"); 
if (!user || response.status === 204) {
  setState("unauthenicated")
  return;
}

If the user only wanted to check for success the "proper" way would be to do

if(response.ok) {
}

anyway.

This is not the end of the world, but caused an unintuitive bug for me that the type system did not catch.
Thanks for your great work on the library!

[drwpow]

Yeah that’s a fair perspective! As you pointed out, I think the current API is conflating data (response body) with ok (2XX code). This is a common pattern proliferated by other popular tools like GraphQL and React Query, but it’s worth digging deeper into how React Query specifically handles 204 responses (since there’s not really such a thing in GraphQL).

And perhaps we just hoist ok to the top level and encourage the same pattern you suggested, but make it even simpler, e.g.:

const { ok, data, error } = api.GET(…)

if (ok) {
  // …
} else {
  // …
}

And just warn everywhere that “data may be undefined for certain 2XX codes.” Anyway, again, looking into other prior art for 204s specifically is needed. But the more I think about {} you’re right the more I think that’s ultimately the wrong API.