Use async iterable webidl type in ReadableStream.from

[lucacasonato]

Ref whatwg/webidl#1397

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • …
• Implementation bugs are filed:
  • Chromium: …
  • Gecko: …
  • WebKit: …
  • Deno: …
  • Node.js: …
• MDN issue is filed: …
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[MattiasBuelens]

Looks good so far! 👍

Once the Web IDL spec change lands, we'll need to update webidl2js to implement the new algorithms for async iterables, and then we can update our reference implementation. Let's keep this PR as a draft until that's sorted out.

[lucacasonato]

@MattiasBuelens Updated!

I am planning to submit a PR to webidl2js, similar to what I've done for widlparser already.

EDIT: PR for webidl2.js: w3c/webidl2.js#775

[lucacasonato]

Moving over the question of whether this API should support async iterable<T> or DOMString, or just async iterable<T> as an argument (from whatwg/webidl#1397 (comment)). In the current streams spec, strings are implicitly allowed because they implement the async iterable protocol. The new async iterable<T> Web IDL type however, only accepts objects that implement the async iterable protocol. The reason for this is because anything else would realistically not be possible due to overloading behaviour in Web IDL. That point is not the point of discussion here.

The question posed is, "Should ReadableStream.from("foobar") be valid, and if so, what should it do?"

I think there are three possible outcomes here:

1. We disallow this behaviour and throw in this case. This mirrors the behaviour of Array.prototype.flatMap of not iterating over strings implicitly.
2. We allow this behaviour, and call [Symbol.iterator] on the string. Each chunk returned from the iterator is enqueued individually into the stream. This mirrors the behaviour of Iterator.from().
3. We allow this behaviour, and enqueue the entire string as one chunk into the stream. This mirrors the behaviour of new Response("foobar").body.

The current behaviour of the spec is option 2. Option 1 would be a more significant breaking change than option 3, but both are unlikely to have web compat issues. I say this because a) ReadableStream.from() is very new, and b) I have not seen any use cases where anyone actually wants to get a stream of individual string chunks.

If we want to support passing strings, option 3 would likely be faster than option 2, because of the lower number of enqueue operations, string allocations, and object allocations involved. It would also mirror the behaviour of new Reponse("foobar").body.

On the other hand, if we do not support strings at all (option 1), users would never be suprised by the choice we made between 1 and 2. Instead, they always get an explicit error that they can then deal with themselves. Both options are still trivially expressible using minimal changes by the developer. If they wanted to emulate the behaviour of option 2, they can do ReadableStream.from(Iterator.from("foobar")), and if they want to emulate option 3, they can do ReadableStream.from(["foobar"]).

My preference is to option 1, followed by option 2 (if we want to support strings at all). I acknowledge that there would then be divergence between new Response("string").body and ReadableStream.from("string"), which is unfortunate, however I think a divergence between the from method of the streaming primitive Iterator/AsyncIterator, and the other streaming primitive ReadableStream would be unfortunate.

cc @domenic @bakkot

[ricea]

My preference is also for option 1.

[saschanaz]

Mozilla has no use counter for string case. I can't imagine the use case either, but might be worth checking first.

[annevk]

@ricea @saschanaz can we take your comments to count as implementer interest for this change? That would also unblock the IDL change.

[ricea]

Yes, you can consider Blink interested.

[saschanaz]

Also LGTM for stream change.

[MattiasBuelens]

Still LGTM, with one small nit.

[saschanaz]

Why export, btw?

[lucacasonato]

Will be needed to resolve whatwg/fetch#1291, which is my next project :)

[saschanaz]

In that case you would want a separate op in section 9, this one is in section 4 and not supposed to be directly used by other specs.

[lucacasonato]

Ok, will revert this here

[annevk]

@lucacasonato any reason this hasn't moved forward?

[lucacasonato]

@annevk I did not have time yet to finish the WebIDL PR - I am doing that now though :)

[MattiasBuelens]

@lucacasonato Do you mind if I pick this up again?

I noticed that the specification changed a little bit: async iterable<T> was renamed to async_sequence<T> in whatwg/webidl#1500. This is mostly a syntactic change, the behavior should remain the same.

I've opened jsdom/webidl2js#278. Once that lands, we can update the reference implementation to let webidl2js handle the conversion.