Re: [whatwg/streams] Specify ReadableStream.[[Transfer]] (#623)

domenic commented on this pull request.

Overall I'm surprised how reasonable this all is even without https://github.com/whatwg/html/issues/935. If you ignore the question of "so how does this work for Indexed DB's use of structured clone", it looks pretty great. I have some question of strategy which I've included in the array.

> In particular, there's an apparent problem with transferring errored streams (namely [[storedError]]), as well as the open question of what even ends up in [[storedError]] if the underlyingSource throws or calls controller.error() it from the original realm with something uncloneable.

This seems similar to the question of what happens if the underlyingSource calls controller.enqueue(somethingUncloneable). The answer is to error the destination stream, right? With a TypeError of some kind I guess.

> StructuredClone doesn't seem to be actually possible to truly polyfill as it iterates over objects in a different order than for-in does

Doesn't it iterate over objects in the same order that `Reflect.ownKeys` does?

> is there any reason StructuredClone isn't/ shouldn't be exposed on its own by the JS engine, for admittedly really niche use cases like this?

That's https://github.com/whatwg/html/issues/793. Basically lack of implementer interest, presumably driven by lack of use cases.

> +
+<h5 id="rs-internal-method-transfer">\[[Transfer]](<var>targetRealm</var>)</h5>
+
+<emu-alg>
+  1. If ! IsReadableStreamLocked(*this*) is *true*, throw a *TypeError* exception.
+  1. If *this*.[[state]] is `"errored"`, throw a *TypeError* exception.
+  1. Let _controller_ be *this*.[[readableStreamController]].
+  1. If _controller_.[[targetRealm]] is *undefined*, throw a *TypeError* exception.
+  1. Let _that_ be a new instance of <a idl>ReadableStream</a> in _targetRealm_.
+  1. Set _that_.[[state]] to  *this*.[[state]].
+  1. Set _that_.[[disturbed]] to *this*.[[disturbed]].
+  1. Set _controller_.[[controlledReadableStream]] to _that_.
+  1. Set _that_.[[readableStreamController]] to _controller_.
+  1. Let _queue_ be _controller_.[[queue]].
+  1. Repeat for each Record {[[value]], [[size]]} _pair_ that is an element of _queue_,
+    1. Set _pair_.[[value]] to ! <a abstract-op>StructuredClone</a>(_pair_.[[value]], _targetRealm_).

This should use ?, not !, as StructuredClone could throw

> @@ -3589,11 +3634,13 @@ throughout the rest of this standard.
 </emu-alg>
 
 <h4 id="enqueue-value-with-size" aoid="EnqueueValueWithSize" throws>EnqueueValueWithSize ( <var>queue</var>,
-<var>value</var>, <var>size</var> )</h4>
+<var>value</var>, <var>size</var>, <var>targetRealm</var> )</h4>

It seems nicer to me if EnqueueValueWithSize stays as a naive implementation of the queue-with-sizes data structure, and the structured cloning happens elsewhere. Is doing that much uglier?

> @@ -12,6 +14,7 @@ const { AcquireWritableStreamDefaultWriter, IsWritableStream, IsWritableStreamLo
 
 const InternalCancel = Symbol('[[Cancel]]');
 const InternalPull = Symbol('[[Pull]]');
+const InternalTranfer = Symbol('[[Transfer]]');

typo Tran[s]fer

> @@ -251,6 +257,33 @@ class ReadableStream {
     const branches = ReadableStreamTee(this, false);
     return createArrayFromList(branches);
   }
+
+  [InternalTranfer](/* targetRealm */) {

I think in code I would reify the realms as their globals.

> @@ -251,6 +257,33 @@ class ReadableStream {
     const branches = ReadableStreamTee(this, false);
     return createArrayFromList(branches);
   }
+
+  [InternalTranfer](/* targetRealm */) {
+    if (IsReadableStreamLocked(this) === true) {
+      throw new TypeError('Cannot transfer a locked stream');
+    }
+    if (this._state === 'errored') {
+      throw new TypeError('Cannot transfer an errored stream');
+    }
+    const controller = this._readableStreamController;
+    if (controller._targetRealm === undefined) {
+      throw new TypeError('Only cloning streams are transferable');
+    }
+    /* can't exactly polyfill realm-transfer */
+    const that = new ReadableStream();

So here this would be `new targetRealm.ReadableStream()`

> @@ -1447,6 +1478,12 @@ Instances of {{ReadableStreamDefaultController}} are created with the internal s
     </tr>
   </thead>
   <tr>
+    <td>\[[targetRealm]]

This approach, of using the ReadableStreamDefaultController in one realm instead of just creating a new stream + controller pair, is very interesting, but not what I expected. Why did you choose that?

To be specific, I would have expected the algorithm to be something like

```js
const reader = this.getReader();

const that = new ReadableStream({
  pull(c) {
    return reader.read().then(
      ({ value, done }) => {
        if (done) {
          c.close();
          return;
        }

        c.enqueue(StructuredClone(value));
      },
      e => c.error(e)
    );
  },
  // probably more
});
```

> @@ -251,6 +257,33 @@ class ReadableStream {
     const branches = ReadableStreamTee(this, false);
     return createArrayFromList(branches);
   }
+
+  [InternalTranfer](/* targetRealm */) {

This would allow you to write web platform tests using iframes (although they might not do so great in our test runner... but maybe they would!)

-- 
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/623#pullrequestreview-11939296