openapi-fetch: add discriminant property to FetchResponse

[OliverJAsh]

Consider the following code:

import createFetchClient from "openapi-fetch";
import type { paths } from "./schema";

const fetchClient = createFetchClient<paths>({
  baseUrl: "https://httpbin.org",
});

const run = async () => {
  const result = await fetchClient.GET("/status/{code}", {
    params: { path: { code: 500 } },
  });
  const { data, error } = result;

  // ❌ This will be evaluated as `false`.
  if (error) {
    console.log("error", error);
  } else {
    // ❌
    // TypeScript has narrowed `data` to a non-nullable type.
    // But, at runtime, `data` is `undefined`.
    console.log("data", data);
  }
};

run();

In this example, openapi-fetch will return an error of value undefined, because content length is 0 and the response is not okay:

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

Lines 205 to 208 in d4689b1

	// handle empty content
	if (response.status === 204 || response.headers.get("Content-Length") === "0") {
	return response.ok ? { data: undefined, response } : { error: undefined, response };
	}

When this happens, the if (error) branch above will be evaluated as false, despite the fact there was actually an error (albeit undefined). (This is also a problem in openapi-react-query.)

TypeScript will then narrow the type of data to a non-nullable type (i.e. exclude undefined). But at runtime, data is actually undefined.

We can mitigate this by avoiding destructuring and using the in operator like so:

const run = async () => {
  const result = await fetchClient.GET("/status/{code}", {
    params: { path: { code: 500 } },
  });

  // ✅ This will be evaluated as `true`.
  if ("error" in result) {
    // ✅
    console.log("error", result.error);
  } else {
    console.log("data", result.data);
  }
};

However, it's a shame to lose destructuring. For this reason I wanted to suggest that openapi-fetch could return a discriminant property in FetchResponse, e.g.:

 export type FetchResponse<T extends Record<string | number, any>, Options, Media extends MediaType> =
   | {
+      responseOk: true;
       data: ParseAsResponse<SuccessResponse<ResponseObjectMap<T>, Media>, Options>;
       error?: never;
       response: Response;
     }
     | {
+      responseOk: false;
       data?: never;
       error: ErrorResponse<ResponseObjectMap<T>, Media>;
       response: Response;
     };

Then, destructuring would be safe again:

const run = async () => {
  const result = await fetchClient.GET("/status/{code}", {
    params: { path: { code: 500 } },
  });

  const { data, error, responseOk } = result;

  // ✅ This will be evaluated as `true`.
  if (!responseOk) {
    // ✅
    console.log("error", error);
  } else {
    console.log("data", data);
  }
};

[drwpow]

Just wanted to add that this technically isn’t a bug; this is intentional behavior as it relates to empty responses (see that PR, and attached issues, for discussion on the topic).

In general, the following is best practice:

const { data, error } = result;
if (error) {
  // show error
} else {
  // 
}

Because it’s highly-unusual to not serve any response whatsoever on server error. It is more acceptable to return an empty response for 2xx codes, but even when POSTing or PATCHing it is generally better to return something.

This meshes philosophically with using OpenAPI in the first place—if you aren’t sending responses to the client you’re missing out on a huge benefit of using OpenAPI in the first place (sure, your requests are still typed, but responses are the lions share of what people use OpenAPI for).

I appreciate the suggestion but at this time we won’t consider adding this, because it:

• only applies to empty responses
• only applies to situations when BOTH your data and error responses are expected to be empty

Designing an API around this edge case isn’t ideal, because you, knowing your schema, could work around this, and/or update your schema and endpoints to return more responses in scenarios where it would make sense to.