Add @@asyncIterator to ReadableStream

[MattiasBuelens]

This PR supersedes #954. So far, I've simply rebased @devsnek's changes onto master and fixed any conflicts.

WPT PR: web-platform-tests/wpt#15097

Closes #778

---

Preview | Diff

[MattiasBuelens]

The WPT submodule hasn't yet been updated, so the tests are currently failing on Travis because of the "unexpected" getIterator method.

The reference implementation passes all of the new tests locally. Even the dreaded Object.prototype check works correctly with the new test runner setup! 😄

[MattiasBuelens]

The tests have found a bug in the spec! 🎉

This test fails with the current reference implementation. When calling s.getReader(), the stream is still locked to the iterator's reader.

The problem is that it.return() is not called whenever a for await..of loop completes normally (i.e. when it.next() resolves with {done: true}). A quick snippet to demonstrate:

function testIterator() {
  return {
    [Symbol.asyncIterator]() {
      return {
        async next() {
          console.log('next() called');
          return {done: true};
        },
        async return() {
          console.log('return() called');
        }
      };
    }
  };
}

(async () => {
  console.log('before');
  for await (const chunk of testIterator()) {
    console.log({chunk});
  }
  console.log('after');
})();

This logs:

before
next() called
after

Clearly, the return() method was not called.

To fix this, we need to change the next() method to release the reader just before returning {done: true}. For example, we could change this line in the reference implementation to the following:

    return ReadableStreamDefaultReaderRead(reader)
      .then(({ value, done }) => {
        if (done) {
          ReadableStreamReaderGenericRelease(reader);
        }
        return ReadableStreamCreateReadResult(value, done);
      });

Or we could re-use the read result (although I don't know if this can cause issues?) to avoid creating an extra object:

    return ReadableStreamDefaultReaderRead(reader, true)
      .then(result => {
        if (result.done) {
          ReadableStreamReaderGenericRelease(reader);
        }
        return result;
      });

Similarly, the spec text should be updated to transform the returned promise with a fulfillment handler.

@domenic @ricea @devsnek Thoughts?

[domenic]

Yep, agreed, we should fix that! This was my second point in #778 (comment) .

[MattiasBuelens]

Yep, agreed, we should fix that! This was my second point in #778 (comment) .

All right, I'll fix it.

Any preference on the style of the .then handler? The only other example where ReadableStreamDefaultReaderRead is used internally in the spec is in step 12 of ReadableStreamTee, where the {done, value} tuple is explicitly destructured and asserted before it's used. Do we need to do the same thing here? For example:

1. Return the result of transforming ! ReadableStreamDefaultReaderRead(reader) with a fulfillment handler which takes the argument result and performs the following steps:
   1. Assert: Type(result) is Object.
   2. Let value be ? Get(result, "value").
   3. Let done be ? Get(result, "done").
   4. Assert: Type(done) is Boolean.
   5. If done is true, perform ! ReadableStreamReaderGenericRelease(reader).
   6. Return ! ReadableStreamCreateReadResult(value, done, true).

Can we just check result.done directly and pass result along as-is (with Object.prototype already in its prototype chain)? Or could that cause issues?

1. Return the result of transforming ! ReadableStreamDefaultReaderRead(reader, true) with a fulfillment handler which takes the argument result and performs the following steps:
   1. Let done be ? Get(result, "done").
   2. If done is true, perform ! ReadableStreamReaderGenericRelease(reader).
   3. Return result.

[ricea]

Any preference on the style of the .then handler? The only other example where ReadableStreamDefaultReaderRead is used internally in the spec is in step 12 of ReadableStreamTee, where the {done, value} tuple is explicitly destructured and asserted before it's used. Do we need to do the same thing here?

I think we have to do it the long way. It would be weird if next() executed Object.prototype.then internally.

[MattiasBuelens]

I've pushed a commit to update WPT to the latest commit of the opened WPT PR, to check whether the new tests also pass on Travis. Once the WPT PR lands, I'll amend the commit to point to the merged commit in WPT.

[MattiasBuelens]

Aaaand the build fails on linter errors. Probably because I had to update eslint to support async function*() {}. But hey, at least npm run wpt succeeded! 😅

I'll see if the linter config can be updated to better match our coding style. If not, I guess I can just eslint --fix and commit. 😛

[MattiasBuelens]

And now it's bikeshed not knowing how to auto-link [@@asyncIterator]()... 🤦‍♂️

[domenic]

LGTM! Will let @ricea do the honors for merging tests and then spec. Thanks so much, @MattiasBuelens and @devsnek!

[zizifn]

Sorry for create noise here, does anyone know timeline for browers plan to support this?

[MattiasBuelens]

Sorry for create noise here, does anyone know timeline for browers plan to support this?

We've filed implementation bugs with all major browser vendors, see #778 (comment). From there, it's up to each individual browser vendor to implement this change as they see fit.

You can star the Chromium issue and/or vote on the Firefox bug in order to let them know that you're interested in this feature. Or, if you're willing to put in the effort, you can contribute an implementation yourself. 😉 (Do post a comment on the bug first to inform them that you're working on it though, so they don't end up doing duplicate work.)