add @@asyncIterator to ReadableStreamDefaultReader #950

devsnek · 2018-08-18T05:17:40Z

Closes #778

probably needs a lot of editorial :)

TimothyGu · 2018-08-21T15:32:41Z

index.bs

+  </tr>
+</table>
+
+<h5 id="default-reader-asynciterator-prototype-next" method for="ReadableStreamDefaultReaderAsyncIteratorPrototype">


This and return() should be <h4>'s.

TimothyGu · 2018-08-21T15:33:39Z

index.bs

@@ -1402,6 +1405,87 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
  1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>

+<h5 id="default-reader-iterator" method for="ReadableStreamDefaultReader">
+  iterator({ cancel = true } = {})


There is no precedent for an iterator method anywhere in the platform and I'm reluctant to start that now. Just having @@asyncIterator is good enough IMO. (@@asyncIterator can take a parameter too.)

iterator() came from the linked issue, i'm unsure what to do here.

Yeah we should have this method. But we should flip it to preventCancel = true. https://w3ctag.github.io/design-principles/#prefer-dict-to-bool third paragraph.

How about "getIterator" or "iterate"? The first provides consistency with the existing "getReader". I do prefer a name with a verb in it.

@domenic The default for this would be preventCancel = false, right?

Hmm, now I'm thinking "values()" for symmetry with sync iterators?

OTOH that has the potential to be confused as a sync iterator…

TimothyGu · 2018-08-21T15:34:58Z

index.bs

+
+<emu-alg>
+  1. Let _O_ be *this* value.
+  1. If Type(_O_) is not Object, throw a *TypeError* exception.


This needs to check if O has a [[Reader]] internal slot. Otherwise O might not be a ReadableStreamDefaultReaderAsyncIterator. It might be worthwhile to have an IsReadableStreamDefaultReaderAsyncIterator abstract operation.

This spec uses this rather "this value" (the ECMA-262 convention). So you could just do Type(this).

Ditto below for cancel.

TimothyGu · 2018-08-21T15:36:19Z

index.bs

+<emu-alg>
+  1. Let _O_ be *this* value.
+  1. If Type(_O_) is not Object, throw a *TypeError* exception.
+  1. Let _reader_ be O.[[Reader]].


Missing _ for O.

TimothyGu · 2018-08-21T15:37:01Z

index.bs

+  1. Let _O_ be *this* value.
+  1. If Type(_O_) is not Object, throw a *TypeError* exception.
+  1. Let _reader_ be O.[[Reader]].
+  1. Let _read_ be ? GetMethod(_reader_, "read").


"read"

Ditto below for cancel.

TimothyGu · 2018-08-21T15:37:37Z

index.bs

+  1. If Type(_O_) is not Object, throw a *TypeError* exception.
+  1. Let _reader_ be O.[[Reader]].
+  1. Let _read_ be ? GetMethod(_reader_, "read").
+  1. Return ? Call(_read_, _reader_);


TimothyGu · 2018-08-21T15:39:06Z

index.bs

+
+<h3 id="default-reader-asynciterator-prototype" interface lt="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+  ReadableStreamDefaultReaderAsyncIteratorPrototype
+</h3>


It should be written somewhere that this inherits from %AsyncIteratorPrototype%.

yea ik... i wasn't sure how/where to do that.

is this a must for implementers? Or is it how things are spec'ed?

@mcollina all async iterators should inherit from (have [[Prototype]] set to) %AsyncIteratorPrototype%, but it doesn't happen by default. i added it in my last commit.

@devsnek reading from https://tc39.github.io/ecma262/#sec-asynciteratorprototype, it does not seem to be mandatory for implementers. I'm just trying to understand if there are any benefits in doing so.

@mcollina Yes, it is mandatory; and yes, there are benefits. Having it would allow

for await (const chunk of stream.getReader().getIterator()) { }

in addition to just

for await (const chunk of stream.getReader()) { }

This is fully consistent with how sync iterators operate, that you can do Array.from(map.entries()) in addition to just Array.from(map).

Additionally, it would allow ECMA-262 to extend %AsyncIteratorPrototype% and the improvements would directly reflect on streams.

~~Also, you can get the %AsyncIteratorPrototype% through Object.getPrototypeOf(Object.getPrototypeOf((async function* () {}).prototype)).~~ as you have found out.

TimothyGu · 2018-08-21T15:39:57Z

index.bs

+</h5>
+
+<div class="note">
+  The <code>iterator</code> method returns an async iterator which can be used to consume the stream. The return method


The return method

TimothyGu · 2018-08-21T15:40:05Z

index.bs

+
+<div class="note">
+  The <code>iterator</code> method returns an async iterator which can be used to consume the stream. The return method
+  of this iterator will optionally release or cancel this reader.


this iterator object

Link release and maybe cancel too?

mcollina · 2018-08-21T17:09:02Z

index.bs

+
+<h3 id="default-reader-asynciterator-prototype" interface lt="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+  ReadableStreamDefaultReaderAsyncIteratorPrototype
+</h3>


is this a must for implementers? Or is it how things are spec'ed?

mcollina · 2018-08-21T17:09:13Z

index.bs

@@ -1402,6 +1405,87 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
  1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>

+<h5 id="default-reader-iterator" method for="ReadableStreamDefaultReader">
+  iterator({ cancel = true } = {})


@domenic The default for this would be preventCancel = false, right?

mcollina · 2018-08-21T17:11:46Z

reference-implementation/lib/readable-stream.js

+    if (O._cancel === true) {
+      const reader = O._reader;
+      const cancel = reader.cancel;
+      cancel.call(reader);


I think it should be return cancel.call(reader);

The spec says "Perform ? Call(…)". There's no return anywhere.

mcollina · 2018-08-21T17:14:07Z

reference-implementation/lib/readable-stream.js

+      cancel.call(reader);
+    }
+  }
+}, Object.getPrototypeOf(Object.getPrototypeOf(async function* () {}).prototype));


What benefit does it add setting the prototype to this?

See #950 (comment).

TimothyGu · 2018-08-21T18:50:12Z

index.bs

+
+<emu-alg>
+  1. Let _iterator_ be ? GetMethod(*this*, "iterator").
+  1. Return ? Call(_iterator_, *this*).


This is not idiomatic and not how existing iterators work in ECMAScript and Web IDL. It in fact should be equal to the iterator method (or whatever it ends up being called). See how [].values === [][Symbol.iterator], Map.prototype.entries === Map.prototype[Symbol.iterator], and URLSearchParams.prototype.entries === URLSearchParams.prototype[Symbol.iterator].

Yeah for spec style we should follow https://tc39.github.io/ecma262/#sec-map.prototype-@@iterator I guess.

agreed, i just wasn't sure how to write that out in bs. thanks for the link.

TimothyGu · 2018-08-21T18:52:50Z

reference-implementation/lib/readable-stream.js

+    if (O._cancel === true) {
+      const reader = O._reader;
+      const cancel = reader.cancel;
+      cancel.call(reader);


This does not implement GetMethod fully. Additionally I'd prefer using Reflect.apply().

We should put a full GetMethod implementation in helpers.js.

TimothyGu · 2018-08-21T18:53:30Z

reference-implementation/lib/readable-stream.js

 }

+const ReadableStreamDefaultReaderAsyncIteratorPrototype = Object.setPrototypeOf({
+  _reader: undefined,
+  _cancel: undefined,


Why does the prototype have these properties? The prototype is not an instance of ReadableStreamDefaultReaderAsyncIterator.

domenic

Great start, and sorry for the delay in reviewing!

domenic · 2018-08-21T19:16:05Z

index.bs

+
+<emu-alg>
+  1. Let _iterator_ be ? GetMethod(*this*, "iterator").
+  1. Return ? Call(_iterator_, *this*).


Yeah for spec style we should follow https://tc39.github.io/ecma262/#sec-map.prototype-@@iterator I guess.

domenic · 2018-08-21T19:17:41Z

index.bs

+
+<h3 id="default-reader-asynciterator-prototype" interface lt="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+  ReadableStreamDefaultReaderAsyncIteratorPrototype
+</h3>


Style nit: this introduces spaces between the > and R and the e and the <. No good. Same throughout various places in the patch.

domenic · 2018-08-21T19:18:55Z

index.bs

+</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReaderAsyncIteratorPrototype(*this*) is *false*, throw a *TypeError* exception.


IsReadableStreamDefaultReaderAsyncIteratorPrototype doesn't seem to be defined?

domenic · 2018-08-21T19:21:59Z

index.bs

+
+<h4 id="default-reader-asynciterator-prototype-return" method for="ReadableStreamDefaultReaderAsyncIteratorPrototype">
+  return()
+</h4>


So apparently async iterator return methods should take a value argument and return a promise for { done: true, value }.

You'll want to return the result of transforming the call to cancel() by a function that creates and returns the { done: true, value } tuple (probably using ReadableStreamCreateReadResult for consistency).

domenic · 2018-08-21T19:22:25Z

reference-implementation/lib/readable-stream.js

@@ -700,8 +700,52 @@ class ReadableStreamDefaultReader {

    ReadableStreamReaderGenericRelease(this);
  }
+
+  iterator({ cancel = true } = {}) {
+    const O = this;


To follow the spec more exactly, don't do the O aliasing. Here and everywhere below.

domenic · 2018-08-21T19:22:51Z

index.bs

+<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]],


This should be !, not ?, I think.

domenic · 2018-08-21T19:25:43Z

reference-implementation/lib/readable-stream.js

+    if (O._cancel === true) {
+      const reader = O._reader;
+      const cancel = reader.cancel;
+      cancel.call(reader);


We should put a full GetMethod implementation in helpers.js.

MattiasBuelens · 2018-08-21T22:17:29Z

index.bs

+</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamDefaultReaderAsyncIteratorPrototype(*this*) is *false*, throw a *TypeError* exception.


next() and return() should reject the returned promise instead of throwing an exception. This also means turning any exception thrown from GetMethod and Call into rejections.

TimothyGu · 2018-08-22T23:00:22Z

index.bs

@@ -1402,6 +1405,92 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
  1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>

+<h5 id="default-reader-iterator" method for="ReadableStreamDefaultReader">
+iterator({ cancel = true } = {})</h5>


Just don't worry about the line length and put it on the same line as <h5>.

TimothyGu · 2018-08-22T23:01:30Z

index.bs

+
+<h5 id="default-reader-@@asynciterator" for="ReadableStreamDefaultReader">[@@asyncIterator]()</h5>
+
+<div class="note">


<p class="note">

TimothyGu · 2018-08-22T23:01:51Z

index.bs

+</div>
+
+The initial value of the <code>@@asyncIterator</code> method is the same function object as the initial value of the
+<code>iterator</code> method.


This should be linked. Also it should be made explicit which iterator method we are talking about (which object it belongs to), if we have more than one in the future.

TimothyGu · 2018-08-22T23:02:16Z

index.bs

@@ -1402,6 +1405,92 @@ lt="ReadableStreamDefaultReader(stream)">new ReadableStreamDefaultReader(<var>st
  1. Perform ! ReadableStreamReaderGenericRelease(*this*).
 </emu-alg>

+<h5 id="default-reader-iterator" method for="ReadableStreamDefaultReader">
+iterator({ cancel = true } = {})</h5>


My point about the naming remains. I still would like to see it as getIterator() or iterate().

TimothyGu · 2018-08-22T23:02:27Z

index.bs

+<h5 id="default-reader-iterator" method for="ReadableStreamDefaultReader">
+iterator({ cancel = true } = {})</h5>
+
+<div class="note">


Use a paragraph element.

TimothyGu · 2018-08-22T23:11:51Z

index.bs

+ <var>x</var> )</h4>
+
+<emu-alg>
+  1. If Type(_x_) is not Object, return *false*.


Per ECMAScript convention, objects usually use a single uppercase letter or a camel-cased phrase as variable name. In this case I believe argument would suffice.

TimothyGu · 2018-08-22T23:12:01Z

index.bs

+
+<emu-alg>
+  1. If Type(_x_) is not Object, return *false*.
+  1. If _x_ does not have a [[Reader]] and [[Cancel]] internal slots, return *false*.


The "a" seems misplaced.

TimothyGu · 2018-08-22T23:12:39Z

index.bs

@@ -1562,6 +1651,16 @@ ReadableStreamBYOBReader(<var>stream</var>)</h4>
  1. Return *true*.
 </emu-alg>

+<h4 id="is-readable-stream-default-reader-asynciterator-prototype">
+aoid="IsReadableStreamDefaultReaderAsyncIteratorPrototype" nothrow>IsReadableStreamDefaultReaderAsyncIteratorPrototype (


You are testing for if an object is an AsyncIterator, not if it's an object.

TimothyGu · 2018-08-22T23:13:12Z

index.bs

+  1. If ! IsReadableStreamDefaultReaderAsyncIteratorPrototype(*this*) is *false*, throw a *TypeError* exception.
+  1. Let _reader_ be *this*.[[Reader]].
+  1. If *this*.[[Cancel]] is *true*, then:
+    1. Let _cancel_ be ? GetMethod(_reader_, `"cancel"`).


You should wrap this ? in a promise too.

TimothyGu · 2018-08-22T23:13:56Z

index.bs

+  </thead>
+  <tr>
+    <td>\[[Prototype]]
+    <td>%AsyncIteratorPrototype%


Any reason why this isn't in a .non-normative?

MattiasBuelens

A few typos to fix. Also wondering whether we should use the Call helper function instead of Reflect.apply.

MattiasBuelens · 2018-08-27T08:27:00Z

reference-implementation/lib/readable-stream.js

+    try {
+      const reader = this._reader;
+      const read = GetMethod(reader, 'read');
+      return Reflect.apply(read, reader, []);


Should we use the Call helper function here? The reference implementation doesn't seem to use Reflect anywhere else, although I'm not sure whether that's a design goal.

MattiasBuelens · 2018-08-27T08:29:12Z

index.bs

+  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_).
+  1. If _result_ is an abrupt completion, return <a>a promise rejected with<a/> _result_.[[Value]].


Typo: change <a/> to </a>

MattiasBuelens · 2018-08-27T08:30:36Z

index.bs

@@ -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>({ preventClose = false })


Typo: change preventClose to preventCancel. Same thing for the next line.

MattiasBuelens · 2018-08-27T08:40:20Z

index.bs

+
+<emu-alg>
+  1. If Type(_argument_) is not Object, return *false*.
+  1. If _argument_ does not have a [[Reader]] and [[PreventCancel]] internal slots, return *false*.


@TimothyGu previously reported this, but it hasn't been tackled yet:

The "a" seems misplaced.

ricea · 2018-08-29T08:58:43Z

I don't understand why a reader object is needed in order to iterate over a stream. I was hoping to be able to write

for await (const chunk of stream) { ... }

and have it do what I mean.

domenic · 2018-08-29T13:52:26Z

I can't remember why I thought using the reader as the entry point was a better idea than using the stream. Maybe explicitness or something? But yeah, probably we should switch it...

devsnek · 2018-08-29T14:19:37Z

Does this work?

When @@asyncIterator of ReadableStream is called when _arguments_, do:

1. Let _getReader_ be ? GetMethod(*this*, `"getReader"`).
2. Let _reader_ be ? Call(_getReader_, *this*).
3. Let _getIterator_ be ? GetMethod(_reader_, @@asyncIterator).
4. Let _iter_ be ? Call(_getIterator_, _reader_, _arguments_).
5. Return _iter_.

MattiasBuelens · 2018-08-29T16:37:24Z

@devsnek That could work... assuming we only add readable[Symbol.asyncIterator](), and not readable.getIterator({ preventCancel }).

If we were to also add readable.getIterator({ preventCancel }) as a shorthand for readable.getReader().getIterator({ preventCancel }), then the stream may end up being locked to a reader with no way to unlock it. Consider:

const iterator = readable.getIterator({ preventCancel: true });
iterator.return();
// readable.locked === true;

Because return() does nothing when preventCancel === true, the stream stays locked to the iterator's reader.

What should happen in this case? We could call reader.releaseLock() from iterator.return(), except we might not be able to release the lock because there are still pending reads:

const iterator = readable.getIterator({ preventCancel: true });
const readPromise = iterator.next();
iterator.return(); // can't unlock, since read is still pending... what now?

I don't know if this is a case we want to support. Perhaps we don't need this, and we only have readable[Symbol.asyncIterator]() which will always either consume the whole stream or cancel it.

devsnek · 2018-08-29T16:44:31Z

@MattiasBuelens i'm honestly not a fan of implicitly acquiring a reader for exactly those reasons.

One thing though -- This feature would depend on ReadableStreamDefaultBYOBReader having an @@asyncIterator, which it does not yet. I think it might be better to hold off on ReadableStream[@@asyncIterator] until byob reader has @@asyncIterator as well.

ricea · 2018-08-30T00:52:19Z

@devsnek The default Reader for a ReadableByteStream is still a ReadableStreamDefaultReader, so we don't need to block on BYOB support. I can't see a way to do bring-your-own-buffer async iterators, as there's no way to supply a buffer to the iterator.

iterator.return(); // can't unlock, since read is still pending... what now?

Can we throw in this case? IIUC, it won't happen when using the for await (...) syntax, only when operating the iterator manually, so it doesn't matter if it's a bit clumsy.

domenic

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.

domenic · 2018-08-30T16:24:39Z

index.bs

+<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.

domenic · 2018-08-30T16:25:42Z

index.bs

+  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.)

domenic · 2018-08-30T16:27:50Z

index.bs

@@ -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.

domenic · 2018-08-30T16:28:37Z

index.bs

+  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

domenic · 2018-08-30T16:28:57Z

index.bs

+<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 !

we don't use ! anywhere else.

domenic · 2018-08-30T16:37:19Z

index.bs

+  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]]

domenic · 2018-08-30T16:38:15Z

index.bs

+    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 !

domenic · 2018-08-30T16:42:18Z

index.bs

+
+<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.

domenic · 2018-08-30T16:43:03Z

index.bs

+    <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.

domenic · 2018-08-30T16:43:14Z

index.bs

+    <td class="non-normative">%AsyncIteratorPrototype%
+  </tr>
+  <tr>
+    <td>\[[Reader]]


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

index.bs

domenic · 2018-08-30T17:03:43Z

OK, here are the big issues left, from what I can see:

Move/alias the methods to ReadableStream, instead of putting them on the default reader.
- Should we alias, or move? I guess alias is nice since it lets you manually consume a couple with a given reader, then switch to for-await-of?
- See the pattern used for cancel() for how this would work. TLDR don't call public API methods, use abstract ops
Handle the return-with-pending-reads case, by returning a rejected promise? This seems like it needs to be handled regardless of where we put the methods.
Should the methods of ReadableStreamDefaultReaderAsyncIteratorPrototype call the public API of the wrapped reader, or should they use the corresponding abstract ops directly?
- I lean toward using the ops directly. Why add the complication of letting patches to ReadableStreamReader mess with how stuff works?
- Then we could get rid of the "if this was an abrupt completion" checks, since e.g. ReadableStreamDefaultReaderRead is nothrow.

domenic · 2018-08-30T17:11:46Z

index.bs

+  1. If _read_ is an abrupt completion, return <a>a promise rejected with</a> _read_.[[Value]].
+  1. Let _result_ be Call(_read_.[[Value]], _reader_).
+  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]].


This is missing auto-release if result.[[Value]]'s done property is set to true, discussed in #778 (comment)

domenic · 2018-08-30T17:12:22Z

index.bs

+    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*).


This is missing auto-release, discussed in #778 (comment) . It should happen in both branches.

ricea · 2018-08-31T10:17:41Z

Should we alias, or move? I guess alias is nice since it lets you manually consume a couple with a given reader, then switch to for-await-of?

But then what would this do?

const reader = stream.getReader();
for await (const chunk of reader) {
  break;
}
reader.read();

Would the read() work? It would seem odd if the reader was unlocked behind the user's back.

If you want to use a reader first, and then iterate, it seems reasonable to wait for your read and then releaseLock(), ie.

const reader = stream.getReader();
const { value, done } = await reader.read();
// do something with first chunk
reader.releaseLock();
for await (const chunk of stream) {
  // do something with rest of the chunks
}

In general, I think having multiple ways to do something tends to lead to confused arguments about which way is "right". So I favour only having the methods on ReadableStream.

devsnek · 2018-08-31T13:25:29Z

So I favour only having the methods on ReadableStream.

but we would still have to acquire and lock, it would just be behind the scenes. I'm okay with an alias but this should definitely be on the reader be sure that's where the data reading happens.

domenic · 2018-08-31T14:03:07Z

I'm okay with an alias but this should definitely be on the reader be sure that's where the data reading happens.

I'm not sure I agree with this reasoning. In general, we provide higher-level utilities on the stream, notably pipeTo() and tee(). Under the scenes, those use readers. But they're intentionally meant to allow consumers to not care about the whole reader paradigm.

In contrast, when you're using a reader, these high-level conveniences are not interesting or necessary---you're doing chunk-wise manual manipulation.

TimothyGu reviewed Aug 21, 2018

View reviewed changes

mcollina reviewed Aug 21, 2018

View reviewed changes

TimothyGu reviewed Aug 21, 2018

View reviewed changes

domenic requested changes Aug 21, 2018

View reviewed changes

MattiasBuelens requested changes Aug 21, 2018

View reviewed changes

TimothyGu reviewed Aug 22, 2018

View reviewed changes

BurtHarris mentioned this pull request Aug 26, 2018

Perf and binary-parse-stream/binary-parser hildjj/node-cbor#86

Closed

MattiasBuelens requested changes Aug 27, 2018

View reviewed changes

MattiasBuelens reviewed Aug 27, 2018

View reviewed changes

add @@asyncIterator to ReadableStreamDefaultReader

7bc6bdc

domenic reviewed Aug 30, 2018

View reviewed changes

Fix my review feedback on superficial stuff

33e2d51

domenic reviewed Aug 30, 2018

View reviewed changes

index.bs Show resolved Hide resolved

Fix build error with issue reference

23ae8f8

domenic reviewed Aug 30, 2018

View reviewed changes

TimothyGu mentioned this pull request Aug 30, 2018

WebStream API support node-fetch/node-fetch#387

Open

devsnek closed this Sep 7, 2018

devsnek deleted the feature/defaultreader-asynciterator branch September 7, 2018 18:33

MattiasBuelens mentioned this pull request Sep 7, 2018

add @@asyncIterator to ReadableStream #954

Closed

This comment has been minimized.

Sign in to view

MattiasBuelens mentioned this pull request Jan 26, 2019

ReadableStream @@asyncIterator web-platform-tests/wpt#15097

Merged

28 tasks


		<h5 id="default-reader-@@asynciterator" for="ReadableStreamDefaultReader">[@@asyncIterator]()</h5>

		<div class="note">

add @@asyncIterator to ReadableStreamDefaultReader #950

add @@asyncIterator to ReadableStreamDefaultReader #950

Conversation

devsnek commented Aug 18, 2018 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

TimothyGu Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiasBuelens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Aug 29, 2018

domenic commented Aug 29, 2018

devsnek commented Aug 29, 2018

MattiasBuelens commented Aug 29, 2018 • edited Loading

devsnek commented Aug 29, 2018

ricea commented Aug 30, 2018

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Aug 18, 2018 •

edited by pr-preview bot

Loading

TimothyGu Aug 21, 2018 •

edited

Loading

TimothyGu Aug 21, 2018 •

edited

Loading

TimothyGu Aug 21, 2018 •

edited

Loading

TimothyGu Aug 21, 2018 •

edited

Loading

MattiasBuelens commented Aug 29, 2018 •

edited

Loading