Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response body should be able to be read after fetching #1159

Closed
1 task done
Raiondesu opened this issue Jun 6, 2023 · 6 comments
Closed
1 task done

Response body should be able to be read after fetching #1159

Raiondesu opened this issue Jun 6, 2023 · 6 comments
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library

Comments

@Raiondesu
Copy link

Raiondesu commented Jun 6, 2023

Description

Currently, if one attempts to read a response body in any way (.text()/.blob()/etc.) after fetching, they will be presented with the following error:

TypeError: Failed to execute ['blob'/'text'/etc.] on 'Response': body stream already read

Reproduction

  1. Set up any openapi-fetch client:
import createClient from 'openapi-fetch';
import type { paths } from './schema'; // generated via openapi-typecript

const client = createClient<paths>({ /* any config */ });
  1. Call json() or blob() in a then block after the request:
client
  .get(''/* any request */, { /* any requestinit */ })
  .then(r => r.response.blob()) // <-- throws TypeError
  1. Run and observe the console:
TypeError: Failed to execute 'blob' on 'Response': body stream already read

Expected result

I argue the user should expect the response property to contain a fully functional original response, with the ability to parse the body without any errors in any way should the need arise.

Checklist

  • I’m willing to open a PR, as I have already implemented a fix using response.clone()
@Raiondesu Raiondesu added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Jun 6, 2023
Raiondesu added a commit to Raiondesu/openapi-typescript that referenced this issue Jun 6, 2023
@drwpow
Copy link
Contributor

drwpow commented Jun 7, 2023

Question: why would you want to get .text() for a typed fetch library for a JSON API? If your schema doesn’t list a JSON response, this library will automatically call .text() for you.

Trying to understand the real-world usecase here to provide a good workaround. I’d like to not have to require everyone calling .json() manually themselves for 99.9% of usecases; IMO that’s a bad API that leads to noisy boilerplate (fine when using the fetch API, but not for a wrapper meant for JSON calls)

@Raiondesu
Copy link
Author

Raiondesu commented Jun 7, 2023

I’d like to not have to require everyone calling .json() manually themselves for 99.9% of usecases;

First of all, I do agree with you on this. However, I'd like to point out that my proposal doesn't require the end user to do anything differently, as the only difference will be the ability to process the body manually.

My usecase (and I assume it's possible I'm not the only one) is to read the blob from the response body. As I stated earlier, trying to do this right now results in an error. Due to this, I had to rely on using the raw fetch() manually, bypassing all the goodies this library provides. I should have included this usecase in the issue for it to be more obvious.

There is also a possibility someone needs to process the raw body stream or the arrayBuffer, hence why I formulated the issue in a more generic way, so that it covers the most amount of usecases.

It is indeed very convenient to assume that 99.9% of usecases will be about parsing JSON, but current implementation also robs the end user of the convenience to also cover the remaining 0.1% by hand if needed. Cloning the response before processing it on the side of the library would allow the end user the flexibility of a native fetch response while still providing all the convenience.

UPD: edited the original issue to include the blob usecase.

@drwpow
Copy link
Contributor

drwpow commented Jun 8, 2023

My usecase (and I assume it's possible I'm not the only one) is to read the blob from the response body.

Could you give more specific detail why you’re trying to read a blob for an endpoint marked as JSON in your OpenAPI schema? I’m looking for any clue your schema would give me that you would want to do this. Not at all assuming your usecase is wrong; I just wanted more information to make a better decision, is all.

As you’re probably already aware, that error comes directly from the native fetch API. because we call json() internally, you can’t then call blob() afterward (at least not without cloning the response). Same as the normal fetch API. So internally, if we know that an endpoint has no application/json content type, this library calls .text() instead. We could add blob() support as well based off content-type hints, but that’s currently a bit of a can of worms.

I’m also not interested in not calling the fetch API’s built-in response.json() method as that could carry a litany of other issues I’m not too versed in to make that decision (aside from an obvious memory leak issue this would introduce).

Anyways, all that said, I’d like to assume that your OpenAPI schema contains all the information needed to infer which response you want, so I’d love additional detail as to why you’d want a blob() for a JSON response. And if an automatic fix weren’t possible a fallback could be adding an option to override the automatic .json() call in exchange for your preferred method (which I’d also like to know which benefit this library provides for you in those cases, so this change can be most helpful to you)

@Raiondesu
Copy link
Author

Raiondesu commented Jun 13, 2023

I believe you have misunderstood my intentions with the implementation a little. I'll try to be a bit more specific.
I propose a simple ~15-line fix, without breaking any currently established logic except getting the native fetch error.

Please, take a look: Raiondesu@92f0412


I’m also not interested in not calling the fetch API’s built-in response.json() method [...]

I understand this and I do agree with you, so this is not what I propose. My proposal involves simply cloning the response before calling .json(). This makes it so the actual response the user gets is indistinguishable from the native fetch response (body is unused), while not sacrificing anything - the user still gets the (possibly) parsed json in the data property exactly as they do now.

[...] I’d love additional detail as to why you’d want a blob() for a JSON response

Besides that the details of my usecase are under an NDA, I believe that they will not be of any value to this discussion anyway, as I am certain that my proposal improves user-experience of the library in general when processing a response, rather than addressing a highly-specific usecase.


P.S.

So internally, if we know that an endpoint has no application/json content type, this library calls .text() instead.

Sorry for the trouble, could you please reference where exactly the library does this?
I'm feeling a bit lost about this: I have searched for any mention of .text() in the code and couldn't find any, so I feel like I must be missing something obvious. I could only find the .json() being called here.

@drwpow drwpow mentioned this issue Jun 16, 2023
3 tasks
@drwpow
Copy link
Contributor

drwpow commented Jun 16, 2023

Thanks for providing more detail, and a great suggestion on implementation. I do think solving this is valuable, as it relates to #1123 and #1157.

I’ve added a new parseAs option in #1169 that allows you to override the default .json() handling if desired.

could you please reference where exactly the library does this?

I apologize; it doesn’t! I was getting confused with some in-flight work / other conversations where we had talked about doing it. But it never ended up landing. But all that will change in the next release.

@drwpow
Copy link
Contributor

drwpow commented Jun 16, 2023

This is fixed in 0.3.0. Thanks again!

@drwpow drwpow closed this as completed Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

No branches or pull requests

2 participants