whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.11k stars 329 forks source link

Allow request take AsyncIterable body? #1291

Open ronag opened 3 years ago

ronag commented 3 years ago

Would be possible/make sense to add AsyncIterable to the extract body algorithm? This would help with interopt with Node streams when implementing fetch in Node without violating the spec.

ricea commented 3 years ago

Maybe.

However, we are going to add ReadableStream.from() to the Streams Standard which will at least make it trivial to construct a body from an AsyncIterable.

jimmywarting commented 3 years ago

Remember asking the exact same question a long time ago, but it got rejected by ReadableStream.from()

https://github.com/whatwg/fetch/issues/809

annevk commented 3 years ago

@jimmywarting I don't think it got rejected, but you suggested to close it in favor of that API. Or am I missing something?

jimmywarting commented 3 years ago

yea, pretty much

mcollina commented 7 months ago

I really think this should be part of the standard. It makes Response so much easier to work with in Node.js, as we need to maintain backward compatibility with Node streams without incurring in the overhead of wrapping one stream type in another.

(Node.js ships this exactly for this reason).

mcollina commented 7 months ago

If this change is welcomed, I’m happy to champion this change and send a PR for it.

annevk commented 7 months ago

In principle I'm supportive of this. The part that seems most tricky is how to make this work IDL-wise.

mcollina commented 7 months ago

The part that seems most tricky is how to make this work IDL-wise.

Some guidance on this would be fantastic.

lucacasonato commented 6 months ago

I have opened a PR to Web IDL with an attempt for how to make this work IDL wise: https://github.com/whatwg/webidl/pull/1397

lucacasonato commented 1 month ago

We shipped a version of this in the wild as an experiment in node compat, and discovered the following issue that the spec will have to deal with that relates to boxed strings:

new Response(new String("hello world"))

Currently, boxed strings are cast to string, because they don't match any of the interface types (ReadableStream, Blob, etc). However if the body type becomes ReadableStream | Blob | BufferSource | DOMString | async iterable<Uint8Array> as proposed, boxed string (which implements [Symbol.iterator]), would be turned into an async iterable and then error on the fact that it yields string chunks, not Uint8Array. This is a web compat issue.

I have two proposed solutions:

  1. Like how we disallow primitive strings casting to async iterable, we also disallow the boxed string being cast to an async iterable. We'd do this in WebIDL, and it would mean that no webidl async iterable argument would allow boxed strings. This would be the simplest.
  2. We add object to the body type union instead of async iterable, and then handle this case in the spec by: checking whether the value is a boxed string, in which case cast to string, otherwise decoding it with the webidl union string | async iterable<Uint8Array>

Node does not seem to exhibit this behaviour because Node does not allow [Symbol.iterable] bodies, only [Symbol.asyncIterble] bodies.

annevk commented 1 month ago

Treating string objects and values in the same way whenever possible seems preferable. Which argues for 1 as otherwise each API would have to deal with this anew. (Also, thanks for experimenting!)

jasnell commented 1 week ago

I had missed it but in Node.js' implementation of Request they do already support passing an async iterator as the body of a request.

async function* foo() { yield 'hello'; }
const req = new Request('http://example.org', { method: 'POST', duplex: 'half', body: foo() } );
console.log(await req.text());

It's a bit unfortunate that this ended up being shipped in Node.js prior to there being support defined in the standard but it is what it is. Unfortunately we (the workers runtime) are starting to have folks request this behavior in our implementation of fetch in the runtime in order to be compatible with Node.js. See https://github.com/cloudflare/workerd/issues/2746

Specifically, users want to be able to pass a Node.js stream.Readable as the body. Since Node.js stream.Readable can be consumed as an AsyncIterable, it works with Node.js' extended behavior. Yes, using ReadableStream.from(nodeReadable) works but is less ergonomic and, unfortunately there are already people in the ecosystem making use of body: nodeReadable as opposed to body: ReadableStream.from(nodeReadable).

This is all a long-winded way of say that I think it would be worthwhile to go ahead and allow body to be an AsyncIterable or Iterable.

mcollina commented 1 week ago

@jasnell, for a purely historical context, we borrowed that behavior from node-fetch to minimize the changes needed by users.

dtinth commented 1 week ago

Bun also supports this non-standard behavior, and by way of documentation (not calling this out as a non-standard feature), potentially implicitly endorsing this pattern.

Also, when I have an AsyncIterable<Uint8Array> and want to convert it into an ArrayBuffer, GitHub Copilot actually suggested this code:

const stream = generateZipStream()
const buffer = await new Response(stream).arrayBuffer()

Admittedly the above code looks a bit cursed, but given that Array.fromAsync wasn't an option back then (and still isn't in Node LTS today, i.e. v20), the above code was a one-liner that works in Node.js (all the way back to v18) and in Bun, and popular enough to be suggested by AI models as a viable solution. With its convenience, I kinda hope it becomes a de-facto standard.

By the way, I later discovered that as of now, the above code does not work in Deno and in browsers. So I had to change it to this, but then compatibility with Node 20 is broken:

const stream = generateZipStream()
const buffer = await new Blob(await Array.fromAsync(stream)).arrayBuffer()

To test if a runtime already supports this feature:

console.log(await new Response((async function* () { yield "Hello, "; yield "world!" })()).text())