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

Use async iterable webidl type in ReadableStream.from #1310

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Mar 25, 2024

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

Copy link
Collaborator

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@lucacasonato
Copy link
Member Author

lucacasonato commented Mar 26, 2024

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

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

ricea commented Mar 29, 2024

My preference is also for option 1.

@saschanaz
Copy link
Member

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

@annevk
Copy link
Member

annevk commented Jul 16, 2024

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

@ricea
Copy link
Collaborator

ricea commented Jul 17, 2024

Yes, you can consider Blink interested.

@saschanaz
Copy link
Member

Also LGTM for stream change.

Copy link
Collaborator

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM, with one small nit.

index.bs Outdated Show resolved Hide resolved
@@ -2123,43 +2123,27 @@ The following abstract operations operate on {{ReadableStream}} instances at a h
</div>

<div algorithm>
<dfn abstract-op lt="ReadableStreamFromIterable" id="readable-stream-from-iterable">
<dfn abstract-op export lt="ReadableStreamFromIterable" id="readable-stream-from-iterable">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export, btw?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will revert this here

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

Successfully merging this pull request may close these issues.

5 participants