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

Allow ReadableStream passthrough #1096

Open
hansottowirtz opened this issue Feb 15, 2021 · 1 comment
Open

Allow ReadableStream passthrough #1096

hansottowirtz opened this issue Feb 15, 2021 · 1 comment

Comments

@hansottowirtz
Copy link

hansottowirtz commented Feb 15, 2021

I'm trying to use this library together with "fetch-mock", for testing some Cloudflare workers. In the workers, I'm using TransformStream and response.body.pipeThrough which work like they would in a Web Worker. (Spec: https://streams.spec.whatwg.org). I'm using https://github.com/MattiasBuelens/web-streams-polyfill to create a ReadableStream which I then pass as the body. However, it's not possible to return a ReadableStream in node-fetch, it always gets transformed to a Buffer. That's because body instanceof Stream returns false here:

} else if (body instanceof Stream) {

This is because this polyfill doesn't use the existing Node streams.

Is it possible to test for streams using body instanceof Stream || typeof body.pipeThrough === 'function'? If necessary I'll create a PR.

Another possibility is to use https://github.com/MattiasBuelens/web-streams-polyfill in order to create response.body on every response in the next version of node-fetch, or to add an additional option to select the stream implementation (web-streams-polyfill or Node streams).
Another option is to add an option sendAsRaw to allow passthrough of any body.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Feb 15, 2021

Is it possible to test for streams using body instanceof Stream || typeof body.pipeThrough === 'function'? If necessary I'll create a PR.

We don't just have to let that slip, we also need to support it so that we can read it. the easiest solution would be to simply convert it to a node stream if it's a whatwg stream using streams.Readable.from(iterable)

Both node streams and whatwg streams are async iterable, but the browsers haven't implemented it yet
the stream polyfill have added async iterable support...


As much as we would like to work with whatwg streams, I guess that in the moment it's internally best if we have a node stream as it works best with piping & passing the data to node's http request

Many node developers depends on response body being a node streams as that is also the way it write stuff back to the fs
res.body.pipe(fs.createWritable('dest.txt'))

So it would be hard to change the response body to something else at this point.

It's much easier to accept both types of stream then it is to return the "correct" things in the response/request
We have already made some tricks to cast some iterable stuff into node stream using streams.Readable.from(iterable) internally (with the most resent one being this PR #1092 that makes it possible to accept a blob that have blob.stream() being either a node, whatwg stream or simply just a iterable object) we could to the same thing if the body passed to us is a whatwg stream and convert it into a node stream using streams.Readable.from


the whatwg streams have a spec proposal to also adding ReadableStream.from(iterator) and it's likely that it will be added


Another thought i had right now is if we could just embraced iterators and looped over them instead
then we would not have to care if it's node, whatwg stream or a iterator and we wouldn't have to "convert" them into a overhead stream by converting whatwg stream or iterators to a node stream and then pipe it

Something potentially better could be to replace

node-fetch/src/body.js

Lines 361 to 383 in 6ee9d31

/**
* Write a Body to a Node.js WritableStream (e.g. http.Request) object.
*
* @param {Stream.Writable} dest The stream to write to.
* @param obj.body Body object from the Body instance.
* @returns {void}
*/
export const writeToStream = (dest, {body}) => {
if (body === null) {
// Body is null
dest.end();
} else if (isBlob(body)) {
// Body is Blob
body.stream().pipe(dest);
} else if (Buffer.isBuffer(body)) {
// Body is buffer
dest.write(body);
dest.end();
} else {
// Body is stream
body.pipe(dest);
}
};

with something like:

	if (isBlob(body)) { 
 		// Body is Blob 
 		body = body.stream();
 	}

   	if (body === null) { 
 		// Body is null 
 		dest.end(); 
 	} else if (Buffer.isBuffer(body)) { 
 		// Body is buffer 
 		dest.write(body); 
 		dest.end(); 
 	} else { 
 		// Body is either a async iterator that yields uint8array, whatwg stream or node stream
		for await (let chunk of body ) {
			if (!dest.write(chunk)) {
				await new Promise(rs => dest.once('drain', rs));
			}
		}
		dest.end()
 	} 

I like this more then what i just did with #1092, but we would have to change some more code throughout the rest of the pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants