Implement async iterables #224
Conversation
domenic
force-pushed
the
async-iterable
branch
7 times, most recently
from
d085aa6
to
93860f4
Compare
|
OK, this is ready for review. I've integration-tested it with whatwg/streams#1035 so the generated code works. But the source code could still benefit from review, and maybe we could pretty up the generated code. And if anyone has better ideas than I put this on top of #225, but I don't think that's a strict dependency. I also tried to separate this out into semi-logical commits to show my progress and separate the parts I'm sure of from the parts I wasn't, but rebaselining the snapshot file is a pain, so that kind of stopped; it's probably best to just review the whole thing. |
README.md
Outdated
|
|
||
| [Asynchronous iterable declarations](https://heycam.github.io/webidl/#idl-async-iterable) require the implementation class to implement the following symbol-named methods, corresponding to algorithms from the Web IDL specification. The `utils` object below refers to the default export from the generated utilities file `utils.js`. | ||
|
|
||
| - `utils.asyncIteratorNext`: corresponds to the [get the next iteration result](https://heycam.github.io/webidl/#dfn-get-the-next-iteration-result) algorithm, and receives a single argument containing an instance of the generated async iterator. For pair asynchronous iterables, the return value must be a `[key, value]` pair array, or `undefined` to signal the end of the iteration. For value asynchronous iterables, the return value must be the value, or `undefined` to signal the end of the iteration. |
There was a problem hiding this comment.
Hmm, this seems kind of bad. Streams can contain undefined values. Probably a spec bug?
There was a problem hiding this comment.
Web IDL doesn't really have a native “undefined” type though, so when the spec says "It must return a Promise that either rejects, resolves with undefined to signal the end of the iteration" in the IDL section (rather than the ES bindings section) I guess it's a bit unclear what exactly this "undefined" refers to. The conversion algorithms for any says "an object reference to a special object that represents the ECMAScript undefined value".
There was a problem hiding this comment.
I'll push a new commit that follows whatwg/webidl#885
|
OK, this is ready for review. It continues to test well with the corresponding whatwg/streams PR. |
| } | ||
| `; | ||
|
|
||
| if (this.iterable.hasReturnSteps) { |
There was a problem hiding this comment.
Hmm, while at the time I thought having an external attribute would be cleaner, I now see that it makes it inconsistent with utils.asyncIteratorInit. I'm open to either this or have return added in the generated file.
There was a problem hiding this comment.
What do you mean by "have return added in the generated file"?
There was a problem hiding this comment.
I meant essentially the other option you provided, which is using utils.asyncIteratorReturn in Impl.prototype to see if we need to add a return method to the prototype. But this looks good too.
There was a problem hiding this comment.
I don't think that works because we only require the Impl class at the bottom of the file, I think for circular dependency reasons. At which time the implementation export might not exist.
lib/constructs/interface.js
Outdated
| } | ||
|
|
||
| if (internal.ongoingPromise) { | ||
| return Promise.reject(new TypeError("return() cannot be called while an ongoing call to next() has not settled")); |
There was a problem hiding this comment.
Question: ongoingPromise is only set to null if the promise next() returns gets fulfilled, not when it gets rejected. It seems to me that we might want the [asyncIteratorReturn] method to still be called (which probably cleans up some system resources) even if the last next() failed…?
There was a problem hiding this comment.
Good catch. I added a test to the streams WPT to catch this, and will send a Web IDL PR to fix it.
| } | ||
| `; | ||
|
|
||
| if (this.iterable.hasReturnSteps) { |
There was a problem hiding this comment.
I meant essentially the other option you provided, which is using utils.asyncIteratorReturn in Impl.prototype to see if we need to add a return method to the prototype. But this looks good too.
This starts to implement async iterables, #215, by following the spec as much as posisble.
This is missing:
Passing arguments to theThis wasn't so bad.utils.asyncIteratorInithook. The hard part here is that I'm not sure how to implement convert arguments for an async iterator method, since all our code to implement conversions and handle argument processing seems to be insidegenerateOverloadConversionswhich is pretty complicated.Adding the
returnmethod appropriately. This is tricky because the Web IDL spec says to only add the return method "If an asynchronous iterator return algorithm is defined for the interface". That is, it's unclear how to generate this code without asking the runtime question ofutils.asyncIteratorReturn in Impl.prototype. The best idea I have here is to add a new extended attribute,[WebIDL2JSHasAsyncIteratorReturnSteps].I would appreciate help here, especially on the former.