Re: [whatwg/streams] Support transferable streams (postMessage) (#1053)

@MattiasBuelens requested changes on this pull request.

Looks pretty good! 🙂

From [your comment on #276](https://github.com/whatwg/streams/issues/276#issuecomment-648287943), I understand that we will not yet support the "double-transfer" case in this first version. Correct?

> There is no reference implementation of this functionality as jsdom does not support transferrable objects, and so it wouldn't be testable.

That is indeed unfortunate.

For [remote-web-streams](https://github.com/MattiasBuelens/remote-web-streams/), I wrote [a mock implementation of `MessageChannel`](https://github.com/MattiasBuelens/remote-web-streams/blob/5ca92e2aee8a0dfbc9bd60b88a524cd46e3fb130/test/mocks/MessageChannel.ts). It doesn't actually do any (de)serialization or send stuff between realms, it's just enough so I can run my tests. But I guess that wouldn't really work for a project like jsdom... 😅 

> +  1. Let |type| be ? [$Get$](|data|, `"type"`).
+  1. Let |value| be ? [$Get$](|data|, `"value"`).
+  1. If ! [$Type$](|type|) is not String, return.
+  1. If |type| is `"pull"`,
+   1. If |backpressurePromise| is not undefined,
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to undefined.
+  1. Otherwise, if |type| is `"error"`,
+   1. Perform ! [$WritableStreamDefaultControllerErrorIfNeeded$](|controller|, |value|).
+   1. If |backpressurePromise| is not undefined,
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to undefined.
+ 1. Add a handler for |port|'s {{MessagePort/messageerror}} event with the following steps:
+  1. Let |error| be a new "{{DataCloneError}}" {{DOMException}}.
+  1. Perform ! [$CrossRealmTransformSendError$](|port|, |error|).
+  1. Perform ! [$WritableStreamDefaultControllerError$](|controller|).

```suggestion
  1. Perform ! [$WritableStreamDefaultControllerError$](|controller|, |error|).
```

> + 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.
+  1. Let |type| be ? [$Get$](|data|, `"type"`).
+  1. Let |value| be ? [$Get$](|data|, `"value"`).
+  1. If ! [$Type$](|type|) is not String, return.
+  1. If |type| is `"pull"`,
+   1. If |backpressurePromise| is not undefined,
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to undefined.
+  1. Otherwise, if |type| is `"error"`,
+   1. Perform ! [$WritableStreamDefaultControllerErrorIfNeeded$](|controller|, |value|).
+   1. If |backpressurePromise| is not undefined,
+    1. [=Resolve=] |backpressurePromise| with undefined.
+    1. Set |backpressurePromise| to undefined.

```suggestion
    1. Set |backpressurePromise| to undefined.
   1. Disentangle |port|.
```
The readable's *cancelAlgorithm* disentangles its port when it sends the `"error"` message. Thus, the writable side should do the same when it receives that message.

> +
+<div algorithm>
+ <dfn abstract-op lt="SetUpCrossRealmTransformReadable">SetUpCrossRealmTransformReadable(|stream|,
+ |port|)</dfn> performs the following steps:
+
+ 1. Perform ! [$InitializeReadableStream$](|stream|).
+ 1. Let |controller| be a [=new=] {{ReadableStreamDefaultController}}.
+ 1. Let |backpressurePromise| be [=a new promise=].
+ 1. Add a handler for |port|'s {{MessagePort/message}} event with the following steps:
+  1. Let |data| be the data of the message.
+  1. If ! [$Type$](|data|) is not Object, return.
+  1. Let |type| be ? [$Get$](|data|, `"type"`).
+  1. Let |value| be ? [$Get$](|data|, `"value"`).
+  1. If ! [$Type$](|type|) is not String, return.
+  1. If |type| is `"chunk"`,
+   1. If ! [$ReadableStreamDefaultControllerCanCloseOrEnqueue$](|controller|) is true,

Is this check still necessary after #1029?

> +    1. Set |backpressurePromise| to [=a new promise=].
+  1. Otherwise, if |type| is `"close"`,
+    1. Perform ! [$ReadableStreamDefaultControllerClose$](|controller|).
+    1. Disentangle |port|.
+  1. Otherwise, if |type| is `"error"`,
+   1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |value|)
+   1. Disentangle |port|.
+ 1. Add a handler for |port|'s {{MessagePort/messageerror}} event with the following steps:
+  1. Let |error| be a new "{{DataCloneError}}" {{DOMException}}.
+  1. Perform ! [$CrossRealmTransformSendError$](|port|, |error|).
+  1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |error|).
+  1. Disentangle |port|.
+ 1. Let |startAlgorithm| be an algorithm that returns undefined.
+ 1. Let |pullAlgorithm| be the following steps:
+  1. Perform ! [$PackAndPostMessage$](|port|, `"pull"`, undefined).
+  1. Return |backpressurePromise|.

In [remote-web-streams](https://github.com/MattiasBuelens/remote-web-streams/), I don't maintain a backpressure promise for `pull()`. [I return nothing](https://github.com/MattiasBuelens/remote-web-streams/blob/5ca92e2aee8a0dfbc9bd60b88a524cd46e3fb130/src/readable.ts#L25) (which gets interpreted as a promise resolved with `undefined`), and instead [I rely](https://github.com/MattiasBuelens/remote-web-streams/blob/5ca92e2aee8a0dfbc9bd60b88a524cd46e3fb130/src/readable.ts#L39-L40) on the fact that `ReadableStreamDefaultControllerEnqueue` will call `pull()` again if needed.

I can see that manually maintaining a backpressure promise makes it more obvious *when* the next pull can happen. On the other hand, it feels like we're duplicating some of the internal pull handling of `ReadableStream` here. For example, in `ReadableStreamTee`, we *do* return an immediately-resolved promise from its *pullAlgorithm*, and rely on the stream to not call pull again until we enqueue a chunk.

-- 
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/1053#pullrequestreview-445080016