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

Thanks for the feedback!

> My feedback so far is that the _cancelAlgorithm_ doesn't seem to follow what seems to be the usual pattern of JS behavior. I think the reason is that you want to use the value returned by the `return` function.

> Further to the above, it also doesn't seem to follow the pattern of the other _cancelAlgorithm_s in the spec, all of those I spotted which either resolve with undefined, or throw a result)

You're right, the resolution value of _cancelAlgorithm_ doesn't matter, because [`ReadableStreamCancel`](https://streams.spec.whatwg.org/commit-snapshots/e9355ce79925947e8eb496563d599c329769d315/#readable-stream-cancel) replaces it with `undefined` anyway.

I looked again at `IteratorClose`. If I read this correctly, then:
1. If there's a `return()` method, try calling it with no arguments.
2. If the original completion was a `throw`, return it.
3. If the completion from calling `return()` (in step 1) was a `throw`, return it (thus *replacing the original completion*).
4. If the return value from `return()` is not an object (i.e. not an iterator result), throw a `TypeError`.
5. Otherwise, return the original completion.

I noticed a couple of things:
* _cancelAlgorithm_ calls `return()` with the given cancel _reason_, whereas `IteratorClose` calls it with no arguments. It turns out that `return()` is only ever called *with* an argument in the context of [`yield *` inside (async) generator functions](https://tc39.es/ecma262/#sec-generator-function-definitions-runtime-semantics-evaluation)... 🤔 So I suppose it makes more sense to leave it out, and align closer to `for..await.of` semantics?
* _cancelAlgorithm_ is missing the check in step 4.

So indeed, as currently specified, _cancelAlgorithm_  is a bit sloppy... 😬 

> Maybe we should just use `IteratorClose` and resolve with undefined?

Ideally, we would use `AsyncIteratorClose`. But as discussed in https://github.com/whatwg/streams/pull/1083#discussion_r520908762, there's an `Await()` in the middle which (we think) wouldn't be allowed in the Streams specification.

We also cannot use `IteratorClose`, because then we would finish closing the iterator too early (since we don't await the promise from `return()`. Also, in step 4 we would be checking if the *promise for the return value* is an object (which it always is), whereas really we need to be checking the *resolution value*.

---

To fix this, we should match the behavior of `AsyncIteratorClose` *exactly*. If we cannot use the abstract operation directly, then I suppose we have to make a copy of it that uses [Web IDL's "react to promise"](https://webidl.spec.whatwg.org/#dfn-perform-steps-once-promise-is-settled) instead of `Await()`? Or could we somehow make `Await()` work? 😕

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1083#issuecomment-1147557078

You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/streams/pull/1083/c1147557078@github.com>