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 request take AsyncIterable body? #1291

Open
Tracked by #1452
ronag opened this issue Aug 11, 2021 · 14 comments
Open
Tracked by #1452

Allow request take AsyncIterable body? #1291

ronag opened this issue Aug 11, 2021 · 14 comments
Labels
addition/proposal New features or enhancements topic: api

Comments

@ronag
Copy link
Contributor

ronag commented Aug 11, 2021

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
Copy link
Collaborator

ricea commented Aug 12, 2021

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
Copy link

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

#809

@annevk
Copy link
Member

annevk commented Aug 30, 2021

@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
Copy link

yea, pretty much

@mcollina
Copy link

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
Copy link

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

@annevk
Copy link
Member

annevk commented Mar 20, 2024

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

@mcollina
Copy link

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

Some guidance on this would be fantastic.

@lucacasonato
Copy link
Member

I have opened a PR to Web IDL with an attempt for how to make this work IDL wise: whatwg/webidl#1397

@lucacasonato
Copy link
Member

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
Copy link
Member

annevk commented Aug 26, 2024

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
Copy link

jasnell commented Oct 10, 2024

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 cloudflare/workerd#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
Copy link

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

@dtinth
Copy link

dtinth commented Oct 10, 2024

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())
  • If supported, it prints "Hello, world!"
  • If unsupported, it prints "[object AsyncGenerator]"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: api
Development

No branches or pull requests

8 participants