Re: [whatwg/streams] ReadableStream.from(asyncIterable) (#1083)

@domenic commented on this pull request.



> @@ -787,6 +795,42 @@ option. If {{UnderlyingSource/type}} is set to undefined (including via omission
      |underlyingSource|, |underlyingSourceDict|, |highWaterMark|, |sizeAlgorithm|).
 </div>
 
+<div algorithm>
+ The static <dfn id="rs-from" method for="ReadableStream">from(|asyncIterable|)</dfn> method steps
+ are:
+
+ 1. Let |stream| be undefined.
+ 1. Let |iteratorRecord| be ? [$GetIterator$](|asyncIterable|, `async`).

```suggestion
 1. Let |iteratorRecord| be ? [$GetIterator$](|asyncIterable|, async).
```

Where the ES spec has special formatting or fonts, web specs ignore them all, generally. (Plus imposing our own formatting for strings.)

> +  1. Let |nextResult| be [$IteratorNext$](|iteratorRecord|).
+  1. If |nextResult| is an abrupt completion, return [=a promise rejected with=]
+     |nextResult|.\[[Value]].
+  1. Let |nextPromise| be [=a promise resolved with=] |nextResult|.\[[Value]].
+  1. Return the result of [=reacting=] to |nextPromise| with the following fulfillment steps,
+     given |iterResult|:
+   1. If [$Type$](|iterResult|) is not Object, throw a {{TypeError}}.
+   1. Let |done| be ? [$IteratorComplete$](|iterResult|).
+   1. If |done| is true:
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](stream.[=ReadableStream/[[controller]]=]).
+   1. Otherwise:
+    1. Let |value| be ? [$IteratorValue$](|iterResult|).
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](stream.[=ReadableStream/[[controller]]=],
+       |value|).
+ 1. Let |cancelAlgorithm| be the following steps, given |reason|:
+  1. Let |returnMethod| be [$GetMethod$](|iteratorRecord|.\[[Iterator]], "`return`").

Hmm, should we grab `return` before cancel time, or delay it as this PR does? The ES spec grabs the `next` method ahead of time... but I guess it usually delays the `"return"` method, so, yeah, this is correct as-is.

> +  static from(asyncIterable) {
+    let stream;
+    const iteratorRecord = GetIterator(asyncIterable, 'async');
+
+    const startAlgorithm = () => undefined;
+
+    function pullAlgorithm() {
+      let nextResult;
+      try {
+        nextResult = IteratorNext(iteratorRecord);
+      } catch (e) {
+        return promiseRejectedWith(e);
+      }
+      const nextPromise = promiseResolvedWith(nextResult);
+      return transformPromiseWith(nextPromise, iterResult => {
+        if (typeof iterResult !== 'object') {

`null` should also throw. There might be a `typeIsObject` predicate in one of the helper files? Or maybe I killed it when we moved stuff to IDL and it became unnecessary.

(Tests would be ideal to catch this case.)

> @@ -34,3 +34,67 @@ exports.TransferArrayBuffer = O => {
 exports.IsDetachedBuffer = O => {
   return isFakeDetached in O;
 };
+
+exports.Call = (F, V, args) => {
+  if (typeof F !== 'function') {
+    throw new TypeError('Argument is not a function');
+  }
+
+  return Function.prototype.apply.call(F, V, args);

Slightly nicer: `Reflect.apply(F, V, args)`

> + 1. Let |startAlgorithm| be the following steps:
+  1. Set |iteratorRecord| to ? [$GetIterator$](|asyncIterable|, async).
+ 1. Let |pullAlgorithm| be the following steps:
+  1. Let |nextResult| be [$IteratorNext$](|iteratorRecord|).
+  1. If |nextResult| is an abrupt completion, return [=a promise rejected with=]
+     |nextResult|.\[[Value]].
+  1. Let |nextPromise| be [=a promise resolved with=] |nextResult|.\[[Value]].
+  1. Return the result of [=reacting=] to |nextPromise| with the following fulfillment steps,
+     given the argument |iterResult|:
+   1. If [$Type$](|iterResult|) is not Object, throw a {{TypeError}}.
+   1. Let |done| be ? [$IteratorComplete$](|iterResult|).
+   1. If |done| is true:
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](stream.[=ReadableStream/[[controller]]=]).
+   1. Otherwise:
+    1. Let |value| be ? [$IteratorValue$](|iterResult|).
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](stream.[=ReadableStream/[[controller]]=],

A future extension seems reasonable to me. Feel free to add a `<!-- comment -->` to the source file to remind us.

That said, it's hard enough to motivate implementers to add convenience features like async iteration in the first place; I worry that doing this in two pieces would reduce the chance of the second piece ever happening. Hmm.

> +     given the argument |iterResult|:
+   1. If [$Type$](|iterResult|) is not Object, throw a {{TypeError}}.
+   1. Let |done| be ? [$IteratorComplete$](|iterResult|).
+   1. If |done| is true:
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](stream.[=ReadableStream/[[controller]]=]).
+   1. Otherwise:
+    1. Let |value| be ? [$IteratorValue$](|iterResult|).
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](stream.[=ReadableStream/[[controller]]=],
+       |value|).
+ 1. Let |cancelAlgorithm| be the following steps:
+  1. Let |returnMethod| be ? [$GetMethod$](|iteratorRecord|.\[[Iterator]], "return").
+  1. If |returnMethod| is undefined, return [=a promise resolved with=] undefined.
+  1. Let |returnResult| be ? [$Call$](|returnMethod|, |iteratorRecord|.\[[Iterator]]).
+  1. Return [=a promise resolved with=] |returnResult|.
+ 1. Set |stream| to ! [$CreateReadableStream$](|startAlgorithm|, |pullAlgorithm|, |cancelAlgorithm|).
+ 1. Return |stream|.

The reason I didn't catch this is because those references don't use `|var|` syntax, so they didn't get highlighted when I clicked step 1's "stream" variable.

> +  }
+  if (typeof func !== 'function') {
+    throw new TypeError();
+  }
+  return func;
+};
+
+exports.GetIterator = (obj, hint = 'sync', method) => {
+  assert(hint === 'sync' || hint === 'async');
+  if (method === undefined) {
+    if (hint === 'async') {
+      method = exports.GetMethod(obj, Symbol.asyncIterator);
+      if (method === undefined) {
+        const syncMethod = exports.GetMethod(obj, Symbol.iterator);
+        const syncIterator = exports.GetIterator(obj, 'sync', syncMethod);
+        return syncIterator; // TODO sync to async iterator

> The ReadableStream.from() implementation already wraps all results from next() and return() in new promises, so we don't really need the sync-to-async adapter for that.

Hmm, interesting. Maybe it shouldn't? But, [`for-await-of`](https://tc39.es/ecma262/#sec-runtime-semantics-forin-div-ofbodyevaluation-lhs-stmt-iterator-lhskind-labelset) does, indirectly, by using Await().

But what is the point of async-from-sync iterator then? I know I introduced it for a reason back when I wrote that spec...

I'll ask around.

> +  1. Let |nextResult| be [$IteratorNext$](|iteratorRecord|).
+  1. If |nextResult| is an abrupt completion, return [=a promise rejected with=]
+     |nextResult|.\[[Value]].
+  1. Let |nextPromise| be [=a promise resolved with=] |nextResult|.\[[Value]].
+  1. Return the result of [=reacting=] to |nextPromise| with the following fulfillment steps,
+     given the argument |iterResult|:
+   1. If [$Type$](|iterResult|) is not Object, throw a {{TypeError}}.
+   1. Let |done| be ? [$IteratorComplete$](|iterResult|).
+   1. If |done| is true:
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](stream.[=ReadableStream/[[controller]]=]).
+   1. Otherwise:
+    1. Let |value| be ? [$IteratorValue$](|iterResult|).
+    1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](stream.[=ReadableStream/[[controller]]=],
+       |value|).
+ 1. Let |cancelAlgorithm| be the following steps:
+  1. Let |returnMethod| be [$GetMethod$](|iteratorRecord|.\[[Iterator]], "`return`").

Yeah, those seem largely designed for `for`-`await`-`of` consumers.

-- 
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/1083#pullrequestreview-534879849