search

whatwg / streams

Streams Standard
https://streams.spec.whatwg.org/
Other
1.35k stars 160 forks source link

Use `async iterable` webidl type in ReadableStream.from #1310

Open lucacasonato opened 6 months ago

lucacasonato commented 6 months ago

Ref https://github.com/whatwg/webidl/pull/1397

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

Preview | Diff

lucacasonato commented 6 months ago

@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: https://github.com/w3c/webidl2.js/pull/775

lucacasonato commented 6 months ago

Moving over the question of whether this API should support async iterable<T> or DOMString, or just async iterable<T> as an argument (from https://github.com/whatwg/webidl/pull/1397#issuecomment-2020386670). 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 commented 6 months ago

My preference is also for option 1.

saschanaz commented 6 months ago

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

annevk commented 2 months ago

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

ricea commented 2 months ago

Yes, you can consider Blink interested.

saschanaz commented 2 months ago

Also LGTM for stream change.