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 is not a ReadableStream when constructed from a string or buffer. #1308

Closed
adamfaulkner-at opened this issue Sep 22, 2021 · 3 comments
Labels

Comments

@adamfaulkner-at
Copy link

Response.body should always be a ReadableStream, however, when a Response is constructed from something else, like a string or a Buffer, a `Buffer is used.

Reproduction

Steps to reproduce the behavior:

Using the REPL:

> var {Response} = require('node-fetch')
undefined
> var r = new Response('asdf');
undefined
> r.body
<Buffer 61 73 64 66>

Expected behavior

The spec says that the body is of type ReadableStream: https://fetch.spec.whatwg.org/#concept-body

So, I'd expect to receive a ReadableStream as the body, instead of a Buffer.

When I try this same snippet of code in a browser, I get the following:

> var r = new Response('asdf')
undefined
> r.body
ReadableStream { locked: false }

Your Environment

software version
node-fetch 2.6.2
node 12.20.1
npm 6.14.10
Operating System MacOS 11.6

Additional context

This is particularly problematic when I'm trying to test my code using fetch-mock, as the default behavior results in Buffers everywhere.

@jimmywarting
Copy link
Collaborator

This have always been like this in v2 due to lack of whatwg:stream support in NodeJS
So we never got around into implementing it and will probably never be supported in v2 as it's maintainess and bug fixes only now.

Beside, response.body have mostly always been a node:stream b/c of this. So what we have done in v3 is that we converted all the kinds of body it gets and normalizes it into a node:stream now (#924)
This makes the transition easier later when we make the switch from node:stream to whatwg:stream in v4

And it's going to be a major breaking change when we do.

So my advice: use v3 if you want to do any conversion hack with Response
the exception is: it will be a node:stream instead so best to use for await (let chunk of body)

@adamfaulkner-at
Copy link
Author

Thanks for the response! It's totally fair that you're not going to fix this in v2.

We'll see about upgrading to v3 in the long term. I'll probably use a wrapper in my code to coerce everything into a stream when constructing a Response.

Putting aside compliance with the spec, It's not clear to me how Response is intended to be used given this bug. Is every function that accepts a Response supposed to inspect the type of the body to determine what to do with it?

@jimmywarting
Copy link
Collaborator

Putting aside compliance with the spec, It's not clear to me how Response is intended to be used given this bug.

The fetch api is built up by so many components that implementing everything at once have been tuff, constructing Responses on your own was not the first priority to get fixed. We didn't count on many doing it. but our goal now is to make a fully spec compliant api. #924 did solve half of the issue by converting/normalizing everything into a node:stream - next switch will be to use web streams instead

Is every function that accepts a Response supposed to inspect the type of the body to determine what to do with it?

For now since both whatwg & node streams are both async iterable that yields uint8arrays then the best option would be to use for..of:

for await (const uint8chunk of res.body) {
  // Do something with uint8array
}

Alternative if you always want the res.body to always be a node stream (even when v4 comes out) is to use stream.Readable.from(res.body) but that will be unnecessary in 3.x

envin3 added a commit to pnetwork-association/ptokens-dapp that referenced this issue Mar 6, 2023
node-fetch don't give the same result if polyfilled with Node.js buffer or web stream.
(See node-fetch/node-fetch#1308 for details)
Vite bundling use polyfill while Webpack web stream: this lead to eosjs not being able to decode the wallet requests.
With cross-node this unreliable behavior is fixed.
envin3 added a commit to pnetwork-association/ptokens-dapp that referenced this issue Mar 6, 2023
node-fetch don't give the same result if polyfilled with Node.js buffer or web stream.
(See node-fetch/node-fetch#1308 for details)
Vite bundling use polyfill while Webpack web stream: this lead to eosjs not being able to decode the wallet requests.
With cross-node this unreliable behavior is fixed.
envin3 added a commit to pnetwork-association/ptokens-dapp that referenced this issue Mar 8, 2023
node-fetch don't give the same result if polyfilled with Node.js buffer or web stream.
(See node-fetch/node-fetch#1308 for details)
Vite bundling use polyfill while Webpack web stream: this lead to eosjs not being able to decode the wallet requests.
With cross-node this unreliable behavior is fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants