-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add async iterable<T>
type to WebIDL
#1397
base: main
Are you sure you want to change the base?
Conversation
This commit lifts the async iterator processing logic from the `ReadableStreamFromIterable` operation in the Streams spec, to a new WebIDL type `async iterable<T>`. This will clean up the streams spec, and enable a change in the Fetch spec to directly allow taking async iterable as request/response bodies, without having to pass them to `ReadableStream.from` first.
Has there been discussion somewhere publicly? 👀 |
@saschanaz No, not of any specifics. The background here is this issue: whatwg/fetch#1291 (comment). @annevk mentioned we'd need to figure out how to make this work IDL wise. This is my attempt at that (it enables updating the typedef (Blob or BufferSource or FormData or URLSearchParams or USVString) XMLHttpRequestBodyInit;
typedef (ReadableStream or XMLHttpRequestBodyInit) BodyInit; to typedef (Blob or BufferSource or FormData or URLSearchParams or USVString) XMLHttpRequestBodyInit;
typedef (ReadableStream or async iterable<BufferSource> or XMLHttpRequestBodyInit) BodyInit; |
Hmm, I see. BTW please include the full PR template: https://github.com/whatwg/webidl/blob/main/PULL_REQUEST_TEMPLATE.md |
I wonder how this ends up working in practice. Say you have |
What is being proposed exactly, to what WebIDL type would it be converted? A reference to an |
@annevk I have specified this behaviour in an update to the overload resolution algorithm in this PR already. The gist is: an I have updated the graphic in the spec that explains this too: In practice:
@petervanderbeken I am proposing the addition of an
If you imagine a matrix of JS stream primitives, with asynchronicity on one axis, and eagerness on the other, this fills out the square opposite of
The use case for this type are Web APIs that take "streams" of values (of unknown length), that they then process in some way. A real Web API that does this right now is
Introduction of this |
Per https://whatpr.org/webidl/1397/402a886...14f1bdb.html#js-async-iterable the result indeed seem to be iterator record. (Adding because I don't think the comment above actually answers this.) |
This design sounds good to me. It seems the Build is currently failing though. Could you write a corresponding PR for Streams to show it in action so we have some assurance it can be adopted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good!
This would technically be a breaking change for That said, I wrote that test more for the sake of completeness, and I don't expect much actual usage in the wild. We can consider making |
@MattiasBuelens I was not aware of this behaviour in Streams. I think we should stick with the behaviour where Let's discuss whether to accept or error on |
@annevk I had already opened a draft PR to the streams spec, showing the integration yesterday. Here is a link: whatwg/streams#1310. |
Do we have Web IDL tests in WPT? I don't think so, right? I guess no tests specifically for this are relevant. Considering this is mostly an editorial refactor, do we even need "implementer interest"? There is nothing to implement until an upstream spec uses this. I'll ask for support anyway though:
Also @jasnell, any thoughts? |
Got some feedback from @saschanaz elsewhere that the constraint that |
We do, but they're weird since they test things via APIs that use those features. https://github.com/web-platform-tests/wpt/tree/master/webidl/ecmascript-binding
I think linking to the tests for streams using this feature would be good. Especially if you can verify that everything you introduce in this PR is covered by some aspect of the streams tests. (Which is probably the case since the streams tests are quite thorough, IIRC.)
Implementer support for Web IDL stuff like this is tricky and requires a judgement call. I think we should CC the bindings teams and give them a chance to weigh in, so @japhet and @yuki3 for Chromium. For example if they were concerned about how to implement this in the bindings layer, or if they were concerned that introducing this feature would be bad because then too many specs might use it and actually it should be rare. However, since this doesn't require any immediate changes to any implementations, I think we shouldn't block on requiring such support, if we don't hear back. |
Regarding strings, I think we should follow the behavior of async iterator helpers's |
The way And since it will not specifically reject strings, and strings have a (Sorry the spec for |
Thanks for the quick response. I'm unsure whether we should think of |
Personally I would specify this by making Plus if you take a string explicitly then you can use it directly instead of having to look up and invoke |
Thanks for the comments @domenic and @bakkot. I agree strongly with @bakkot that in the Web IDL type, we should only allow objects implementing the async iterable protocol for the For the streams specific discussion, lets move it right here: whatwg/streams#1310 (comment) |
Folks, could you take another look? I have clarified the allowed positions for async iterables (namely not in attributes), and have added guidance that: @annevk has confirmed implementer interest for WebKit on Matrix - is anyone else in favor, or opposed? |
This LGTM |
Gently pushing for reviews again :) Still need positions from Chromium (@domenic) and Mozilla (@saschanaz). |
TC39 is considering rejecting strings in most (new) iterable-taking positions in general (and of course async iterable taking positions as well). It'll be discussed at the plenary at the end of the month, and I'm guessing it'll be adopted. |
@lucacasonato coupling this with the Streams PR I'm happy to consider Chromium and WebKit as interested and nobody opposed. This PR needs rebasing, the checklist in OP needs a bit more work, and maybe @MattiasBuelens can do a final pass after that. With that I think we're good to go. |
I'd like to ask @petervanderbeken for the position as he will be the implementer. A parallel question for distinguishability: currently it says it's distinguishable with interfaces and dictionaries, but what happens if the interface has |
@saschanaz The interface can be distinguished from other objects because it is branded. A union of Union resolution goes like this in this case: `MyInterface | MyDictionary | async iterable (assuming V has been checked to be an object):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! I still have a question about what it really means to "convert an IDL async iterable value to a JavaScript object" though, see below.
index.bs
Outdated
<div id="async-iterable-to-js" algorithm="convert an async iterable to a JavaScript value"> | ||
An IDL <a lt="async iterable type">async iterable<|<var ignore>T</var>></a> value is | ||
[=converted to a JavaScript value|converted=] to a JavaScript object as follows: | ||
|
||
1. Return the JavaScript object that represents the same async iterable as the IDL value. | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we need this? If I understand correctly, IDL interfaces should always add an async iterable declaration to themselves, rather than returning an async iterable from one of its operations.
Also, if the async iterable value can only have been created by converting it from a JavaScript value, is this value still usable when converting it back to a JavaScript value? The previous algorithm calls GetIterator(V, async)
, which for a ReadableStream
locks the stream. When we return the ReadableStream
back to JavaScript, it'll still be locked by IDL and JavaScript won't be able to async-iterate the stream.
Would it be an option for us to just disallow converting an IDL async iterable type back to a JavaScript value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe the real problem is that the IDL machinery should not be calling GetIterator()
from within the conversion logic in the first place. 🤔 For example: if an IDL operation accepts two async iterable<T>
values as parameters, and GetIterator()
succeeds for the first value but fails for the second value (e.g. because it's a locked ReadableStream
), then the operation errors immediately with a TypeError
but the first value is now permanently "locked".
I think we should make "convert from a JavaScript object" not call GetIterator()
:
- Make an IDL async iterable only represent a JavaScript object.
- Add a separate type for an "IDL async iterator", which is represented by a JavaScript
Iterator
record. - Add an "open an async iterable" algorithm, which takes an IDL async iterable, calls
GetIterator()
and returns it as an IDL async iterator. - Refactor the "get the next value of an async iterable" and "close an async iterable" algorithms to work on an IDL async iterator instead.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be an option for us to just disallow converting an IDL async iterable type back to a JavaScript value?
That was indeed my intention, except for the case where the value just has to pass through IDL momentarially (like in functions like setTimeout
, that call a passed callback with passed arguments). I am not sure now whether this is needed, but earlier reading of the spec made me beleive that all IDL values had to have a way to convert back to JS for these cases - maybe I am wrong.
I think we should make "convert from a JavaScript object" not call GetIterator():
That seems reasonable. I think the converter should do the following:
- Check
Type(V)
isObject
- Check that
V
has aSymbol.asyncIterator
orSymbol.iterator
property, and if so, store it. (But do not call it yet.)
Then the open an async iterable
algorithm:
- Calls the iterator method stored during the conversion (using ECMA262's
GetIteratorFromMethod
) - Returns the ECMA262's
Iterator
record (or an IDL wrapper thereof)
The other async iterable algorithms are then modified to work with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MattiasBuelens I have attempted to implement this in 5533589.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! Just a few small typos left, otherwise LGTM.
This commit lifts the async iterator processing logic from the
ReadableStreamFromIterable
operation in the Streams spec, to a new WebIDL typeasync iterable<T>
.This will clean up the streams spec, and enable a change in the Fetch spec to
directly allow taking async iterable as request/response bodies, without users
having to pass them to
ReadableStream.from
first.async iterable<T>
type w3c/webidl2.js#775async iterable<T>
type plinss/widlparser#85(See WHATWG Working Mode: Changes for more details.)
Preview | Diff