Re: [whatwg/streams] add @@asyncIterator to ReadableStreamDefaultReader (#950)

domenic commented on this pull request.

First round of review. Here I address superficial issues. I'll upload a patch fixing all of these as I go, as penance for the long turnaround times so far.

> @@ -1402,6 +1405,90 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
   1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>
 
+<h5 id="default-reader-getiterator" method for="ReadableStreamDefaultReader">getIterator({ preventCancel = false } = {})
+</h5>
+
+<p class="note">
+  The <code>getIterator</code> method returns an async iterator which can be used to consume the stream. The
+  {{ReadableStreamDefaultReaderAsyncIteratorPrototype/return()}} method of this iterator object will optionally
+  {{ReadableStreamDefaultReader/cancel()}} this reader.

We should talk about canceling the stream, not link to the public API on reader that does so.

We should also mention the auto-release behavior.

> @@ -1402,6 +1405,90 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
   1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>
 
+<h5 id="default-reader-getiterator" method for="ReadableStreamDefaultReader">getIterator({ preventCancel = false } = {})
+</h5>
+
+<p class="note">
+  The <code>getIterator</code> method returns an async iterator which can be used to consume the stream. The
+  {{ReadableStreamDefaultReaderAsyncIteratorPrototype/return()}} method of this iterator object will optionally
+  {{ReadableStreamDefaultReader/cancel()}} this reader.
+</p>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReader(*this*) is *false*, throw a *TypeError* exception.
+  1. If *this*.[[ownerReadableStream]] is *undefined*, throw a *TypeError* exception.
+  1. Let _iterator_ be ObjectCreate(`<a idl>ReadableStreamDefaultReaderAsyncIteratorPrototype</a>`, « [[Reader]],
+  [[PreventCancel]] »).

Interesting. Elsewhere in the spec we fail to add the internal slots. For consistency let's remove that here, and file a separate issue to fix it. (Although I'd like a fix that is less verbose and annoying than the ES spec's repetition of the internal slots list.)

> @@ -1283,6 +1283,9 @@ would look like
     <a href="#default-reader-cancel">cancel</a>(reason)
     <a href="#default-reader-read">read</a>()
     <a href="#default-reader-release-lock">releaseLock</a>()
+
+    <a href="#default-reader-getiterator">getIterator</a>({ preventCancel = false })

In both the algorithm and the header, we should follow the pattern of pipeTo (and other web APIs), of not defaulting to false, but instead doing ToBoolean on the argument.

> +<h5 id="default-reader-getiterator" method for="ReadableStreamDefaultReader">getIterator({ preventCancel = false } = {})
+</h5>
+
+<p class="note">
+  The <code>getIterator</code> method returns an async iterator which can be used to consume the stream. The
+  {{ReadableStreamDefaultReaderAsyncIteratorPrototype/return()}} method of this iterator object will optionally
+  {{ReadableStreamDefaultReader/cancel()}} this reader.
+</p>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReader(*this*) is *false*, throw a *TypeError* exception.
+  1. If *this*.[[ownerReadableStream]] is *undefined*, throw a *TypeError* exception.
+  1. Let _iterator_ be ObjectCreate(`<a idl>ReadableStreamDefaultReaderAsyncIteratorPrototype</a>`, « [[Reader]],
+  [[PreventCancel]] »).
+  1. Set _iterator_.[[Reader]] to *this*.
+  1. Set _iterator_.[[PreventCancel]] to ! ToBoolean(_cancel_).

_cancel_ -> _preventCancel_

> @@ -1402,6 +1405,90 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
   1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>
 
+<h5 id="default-reader-getiterator" method for="ReadableStreamDefaultReader">getIterator({ preventCancel = false } = {})
+</h5>
+
+<p class="note">
+  The <code>getIterator</code> method returns an async iterator which can be used to consume the stream. The
+  {{ReadableStreamDefaultReaderAsyncIteratorPrototype/return()}} method of this iterator object will optionally
+  {{ReadableStreamDefaultReader/cancel()}} this reader.
+</p>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReader(*this*) is *false*, throw a *TypeError* exception.
+  1. If *this*.[[ownerReadableStream]] is *undefined*, throw a *TypeError* exception.
+  1. Let _iterator_ be ObjectCreate(`<a idl>ReadableStreamDefaultReaderAsyncIteratorPrototype</a>`, « [[Reader]],

ObjectCreate should not fail ,so add !

> +  1. Set _iterator_.[[Reader]] to *this*.
+  1. Set _iterator_.[[PreventCancel]] to ! ToBoolean(_cancel_).
+  1. Return _iterator_.
+</emu-alg>
+
+<h5 id="default-reader-@@asynciterator" for="ReadableStreamDefaultReader">[@@asyncIterator]()</h5>
+
+<p class="note">
+  The <code>@@asyncIterator</code> method is an alias of {{ReadableStreamDefaultReader/getIterator()}}.
+</p>
+
+The initial value of the <code>@@asyncIterator</code> method is the same function object as the initial value of the
+{{ReadableStreamDefaultReader/getIterator()}} method.
+
+<h3 id="default-reader-asynciterator-prototype" interface lt="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+ReadableStreamDefaultReaderAsyncIteratorPrototype</h3>

This introduces whitespace between the `>` and the `R`, so this is not the right place to wrap.

> +<h5 id="default-reader-@@asynciterator" for="ReadableStreamDefaultReader">[@@asyncIterator]()</h5>
+
+<p class="note">
+  The <code>@@asyncIterator</code> method is an alias of {{ReadableStreamDefaultReader/getIterator()}}.
+</p>
+
+The initial value of the <code>@@asyncIterator</code> method is the same function object as the initial value of the
+{{ReadableStreamDefaultReader/getIterator()}} method.
+
+<h3 id="default-reader-asynciterator-prototype" interface lt="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+ReadableStreamDefaultReaderAsyncIteratorPrototype</h3>
+
+<h4 id="default-reader-asynciterator-prototype-internal-slots">Internal slots</h4>
+
+{{ReadableStreamDefaultReaderAsyncIteratorPrototype}} is created with the internal slots described in the following
+table:

I'm not super-comfortable with describing the internal slots of prototypes this way. Let me try something different.

In particular, [[Prototype]] is an internal slot of ReadableStreamDefaultReaderAsyncIteratorPrototype, whereas the other two are slots on the instances.

> +    <tr>
+      <th>Internal Slot</th>
+      <th>Description (<em>non-normative</em>)</th>
+    </tr>
+  </thead>
+  <tr>
+    <td>\[[Prototype]]
+    <td class="non-normative">%AsyncIteratorPrototype%
+  </tr>
+  <tr>
+    <td>\[[Reader]]
+    <td class="non-normative">A {{ReadableStreamDefaultReader}} instance.
+  </tr>
+  <tr>
+    <td>\[[PreventCancel]]
+    <td class="non-normative">Boolean value indicating if the reader will be cancelled when the stream returns.

It's the stream that is canceled, not the reader. And it's not the stream that returns, it's the async iterator.

> @@ -1562,6 +1649,16 @@ ReadableStreamBYOBReader(<var>stream</var>)</h4>
   1. Return *true*.
 </emu-alg>
 
+<h4 id="is-readable-stream-default-reader-asynciterator-prototype">
+aoid="IsReadableStreamDefaultReaderAsyncIterator" nothrow>IsReadableStreamDefaultReaderAsyncIterator (

Markup got mangled. Look at the build result!!

> +  </tr>
+  <tr>
+    <td>\[[PreventCancel]]
+    <td class="non-normative">Boolean value indicating if the reader will be cancelled when the stream returns.
+  </tr>
+</table>
+
+<h4 id="default-reader-asynciterator-prototype-next" method for="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+next()</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReaderAsyncIterator(*this*) is *false*, throw a *TypeError* exception.
+  1. Let _reader_ be *this*.[[Reader]].
+  1. Let _read_ be GetMethod(_reader_, `"read"`).
+  1. If _read_ is an abrupt completion, return <a>a promise rejected with</a> _read_.[[Value]].
+  1. Let _result_ be Call(_read_, _reader_).

Missing .[[Value]]

> +  1. If _result_ is an abrupt completion, return <a>a promise rejected with</a> _result_.[[Value]].
+  1. Return <a>a promise resolved with</a> _result_.[[Value]].
+</emu-alg>
+
+<h4 id="default-reader-asynciterator-prototype-return" method for="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+return( <var>value</var> )</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReaderAsyncIterator(*this*) is *false*, throw a *TypeError* exception.
+  1. Let _reader_ be *this*.[[Reader]].
+  1. If *this*.[[PreventCancel]] is *false*, then:
+    1. Let _cancel_ be GetMethod(_reader_, `"cancel"`).
+    1. If _cancel_ is an abrupt completion, return <a>a promise rejected with</a> _cancel_.[[Value]].
+    1. Let _result_ be Call(_cancel_.[[Value]], _reader_, &laquo; _value_ &raquo;).
+    1. If _result_ is an abrupt completion, return <a>a promise rejected with</a> _result_.[[Value]].
+  1. Return <a>a promise resolved with</a> ? ReadableStreamCreateReadResult(_value_, *true*, *true*).

ReadableStreamCreateReadResult cannot fail, so use !

> @@ -1562,6 +1649,16 @@ ReadableStreamBYOBReader(<var>stream</var>)</h4>
   1. Return *true*.
 </emu-alg>
 
+<h4 id="is-readable-stream-default-reader-asynciterator-prototype">
+aoid="IsReadableStreamDefaultReaderAsyncIterator" nothrow>IsReadableStreamDefaultReaderAsyncIterator (
+ <var>argument</var> )</h4>
+
+<emu-alg>
+  1. If Type(_argument_) is not Object, return *false*.
+  1. If _argument_ does not have [[Reader]] and [[PreventCancel]] internal slots, return *false*.

We only generally test one slot.

> +{{ReadableStreamDefaultReaderAsyncIteratorPrototype}} is created with the internal slots described in the following
+table:
+
+<table>
+  <thead>
+    <tr>
+      <th>Internal Slot</th>
+      <th>Description (<em>non-normative</em>)</th>
+    </tr>
+  </thead>
+  <tr>
+    <td>\[[Prototype]]
+    <td class="non-normative">%AsyncIteratorPrototype%
+  </tr>
+  <tr>
+    <td>\[[Reader]]

For no good reason, we use lowercase slot names in this spec. We have an open bug to fix, but we should align until then.

> +{{ReadableStreamDefaultReaderAsyncIteratorPrototype}} is created with the internal slots described in the following
+table:
+
+<table>
+  <thead>
+    <tr>
+      <th>Internal Slot</th>
+      <th>Description (<em>non-normative</em>)</th>
+    </tr>
+  </thead>
+  <tr>
+    <td>\[[Prototype]]
+    <td class="non-normative">%AsyncIteratorPrototype%
+  </tr>
+  <tr>
+    <td>\[[Reader]]

We should have a slot name that's more distinguishable for the brand check.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/950#pullrequestreview-151077224