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

Add async_iterable support #720

Merged
merged 3 commits into from
Aug 7, 2019
Merged

Add async_iterable support #720

merged 3 commits into from
Aug 7, 2019

Conversation

Ms2ger
Copy link
Member

@Ms2ger Ms2ger commented May 1, 2019

Fixes #580.


Preview | Diff

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

High-level comments as I skim through this between meetings. Will do more later.

@@ -4091,6 +4104,138 @@ The following extended attributes are applicable to [=iterable declarations=]:
</pre>


<h4 id="idl-async-iterable">Asynchronously iterable declarations</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I've never seen it spelled out as "asynchronously iterable". Maybe an "asynchronous iterable", as in the title of https://github.com/tc39/proposal-async-iteration. But "async iterable" is much more common.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Overall LGTM! A few clarity suggestions.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@bzbarsky
Copy link
Collaborator

This could probably use review from other implementors before it merges....

@domenic
Copy link
Member

domenic commented Jun 19, 2019

@bzbarsky would you have time to review this soon? I am planning to land WICG/kv-storage#68 with a reference to this pull request, but it would be even better if I could reference the base spec.

@bzbarsky
Copy link
Collaborator

This week is a Mozilla all-hands, so my time is very unpredictable through Friday. I can make this a priority on Monday, though. Is that good enough?

@domenic
Copy link
Member

domenic commented Jun 19, 2019

That would be great, thank you!

@ricea
Copy link
Contributor

ricea commented Jun 25, 2019

I have some concerns about how we will apply this to the Streams standard. The iterable definition has this text:

If a single type parameter is given, then the interface has a value iterator and provides values of the specified type

but async_iterable lacks a similar provision. Since a ReadableStream has no keys, this makes application of the standard text difficult. There's nothing meaningful that a keys() method could iterate over, and I don't know how values() would be different from entries().

@Ms2ger
Copy link
Member Author

Ms2ger commented Jun 25, 2019

I've got a WIP for value-only iterators in https://github.com/heycam/webidl/compare/async-iterator-values?expand=1.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
1. a value of the second type given in the declaration;
1. an opaque value that is passed back to the next invocation of the algorithm,

The prose may also define <dfn>asynchronous iterator initialization steps</dfn> for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these steps allowed to do? This seems pretty open-ended... Maybe that's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is supposed to be a step towards handling https://streams.spec.whatwg.org/#rs-get-iterator ; not sure if it's necessary to restrict anything here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we OK with the steps here calling into arbitrary script, say? (We might be; I haven't audited very carefully.)

index.bs Outdated

<pre class="grammar" id="prod-AsyncIterable">
AsyncIterable :
"async_iterable" "&lt;" TypeWithExtendedAttributes "," TypeWithExtendedAttributes "&gt;" ";"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for async_iterable rather than async iterable? The latter seems a bit nicer to read/write to me and the grammar bit should be fine, I expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

No strong feelings from my side on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would personally prefer async iterable.

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

Ms2ger commented Jun 26, 2019

Thanks for the review; I think I covered most of the issues.

@bzbarsky
Copy link
Collaborator

Generally looks good, thanks! I think I would still prefer async iterator without the underscore.

@ricea
Copy link
Contributor

ricea commented Jun 28, 2019

Generally looks good, thanks! I think I would still prefer async iterator without the underscore.

@yuki3 Would async iterator as two tokens cause problems for Blink's IDL parser?

@yuki3
Copy link

yuki3 commented Jun 28, 2019

I think that it should be just fine. We already have "partial dictionary", "partial interface", "callback interface", etc. I don't think that a space between "async" and "iterator" will be a problem.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Shall we merge?

@inexorabletash
Copy link
Member

What's the support in bikeshed and WPT's respective IDL parsers?

@marcoscaceres
Copy link
Member

For WTP, I don't think we support this yet in WebIDL2.js, right @saschanaz? Would be trivial to add tho.

@saschanaz
Copy link
Member

@marcoscaceres Right, we don't. I can cook a PR soon.

@saschanaz
Copy link
Member

Just to make sure: async_iterable or async iterable?

@marcoscaceres
Copy link
Member

"async iterable" by looking at the example.

@marcoscaceres
Copy link
Member

Ok, @saschanaz added support (w3c/webidl2.js#365). We will wait for this to merge. Probably want to bugs filed on browser engines too.

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.

Async iterators
8 participants