fix(#1977): refactor handling for empty responses

[DjordyKoert]

Changes

This PR includes a couple of different changes for openapi-fetch:

• A refactor (Breaking change?) to ensure the behaviour of data and error are the same to reduce complexity.
  As an example, this means that whenever a user uses the parseAs: 'stream' both the data and error property will return a ReadableStream from response.body
• The logic for empty response bodies has been changed:
  • It no longer explicity checks for 204 status to determine if a response is empty
  • It now follows the spec by checking if response.body is null
    • Backwards compatibality with browsers: empty string ('') get converted to undefined after failing to parse it as json (fixes Unexpected end of JSON input with empty response #1977)

How to Review

See changes & tests

Checklist

• Unit tests updated
• docs/ updated (if necessary)
• pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot]

🦋 Changeset detected

Latest commit: 0345a28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
openapi-fetch	Patch
openapi-react-query	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[drwpow]

I think this is a breaking change, but one most seem to be in favor of. Love the direction of this so far! Please let us know if you need any assistance with passing tests, or have any questions.

[gzm0]

Looking at #1970, I have been thinking about the "parse-as" problem a bit as well.

I'm wondering if it would make sense to make the parsing lazier. One issue I see with the current API is that the caller cannot look at the status code to decide on how to parse the response. I'm not sure this is very ergonomic.

For example, it is very possible that an API returns a binary stream on 200, but a JSON response on other error codes. In this case, being able to specify parseAs only individually might not be sufficient.

One way to work around this problem is to provide accessors:

const resp = client.GET("/thing");

if (resp.status == 200) {
  resp.stream()
} else {
  resp.json()
}

This is somewhat inspired by requests.

I realize

• this wouldbe a major change
• we need to be careful about the performance implications of the potentially additional wrapper we'd need

However, I fear if we go down the "pass parseAs parameter path", we'll get more and more complicated options (e.g. parseAs per response code, etc.).

[drwpow]

@gzm0 I completely agree—parseAs is not an ideal solution; it’s just a bandaid to not introduce performance issues. Many people find it hard to use, and want it to be more automatic.

This is related to some longer conversations we maintainers have had where some part of openapi-fetch’s runtime is code-generated from peoples’ specs—that would remove the need for parseAs in exchange for bigger client weight and slightly-more memory. That’s of course a completely separate conversation, but not entirely unrelated. I do think that decision of “does openapi-fetch continue on its current path of requiring zero codegen beyond openapi-typescript” will likely affect the decisions on the parseAs API. So we should probably set that in stone once and for all to help guide this decision better.

[gzm0]

I do think that decision of “does openapi-fetch continue on its current path of requiring zero codegen beyond openapi-typescript” will likely affect the decisions on the parseAs API. So we should probably set that in stone once and for all to help guide this decision better.

Agreed.

FWIW: Have we considered simply returning the fetch Response object with a more narrowed type directly?

That would allow something like:

const response = await client.GET("/path");

if (!response.ok) { // type narrowing on both `ok` and `status` might be tricky but probably necessary for ergonomics.
  throw new Error("foo");
}

const data = await response.json();

While all the lower-level body retrieval methods of Response remain available.

[drwpow]

Have we considered simply returning the fetch Response object with a more narrowed type directly?

That was one of the earliest decisions I made to just save hundreds of .json()s sprinkling peoples’ code just for the few times when you needed something else (with the assumption OpenAPI is 99% JSON-centric). I’m open to an argument to have a lower-level API if we feel it’s a more cohesive design. I think this would be worth an RFC to get feedback from everyone if that’s a solution to these problems (which I think it would be)

[gzm0]

That was one of the earliest decisions I made to just save hundreds of .json()s sprinkling peoples’ code just for the few times when you needed something else (with the assumption OpenAPI is 99% JSON-centric).

Yeah, that's fair.

I’m open to an argument to have a lower-level API if we feel it’s a more cohesive design. I think this would be worth an RFC to get feedback from everyone if that’s a solution to these problems (which I think it would be)

Fair enough. So it really seems this warrants wider discussion.