Re: [whatwg/streams] Add support for ReadableStream "owning" type (PR #1271)

@saschanaz requested changes on this pull request.

The idea looks fine to me. Some nits:

> @@ -17,12 +17,16 @@ exports.implementation = class ReadableStreamDefaultControllerImpl {
     aos.ReadableStreamDefaultControllerClose(this);
   }
 
-  enqueue(chunk) {
+  enqueue(chunk, options) {
+    const transferList = options ? options.transfer : [];

I'm not very familiar with this reference implementation, but:

1. If webidl binding works here, then `options` cannot be undefined here.
2. If not, this should also care for the case when `options.transfer` is undefined.

> +<div algorithm>
+ <dfn abstract-op lt="StructuredTransferOrClone">StructuredTransferOrClone(|value|, |transferList|)</dfn>
+    performs the following steps:
+ 1. Let |serialized| be ? [$StructuredSerializeWithTransfer$](|value|, |transferList|).
+ 1. Let |deserialized| be ? [$StructuredDeserializeWithTransfer$](|serialized|, [=the current Realm=]).
+ 1. Return |deserialized|.\[[Deserialized]].
+</div>
+

This is a copy of https://html.spec.whatwg.org/multipage/structured-data.html#dom-structuredclone. Should we ask HTML to export these steps?

> @@ -2950,13 +2969,21 @@ The following abstract operations support the implementation of the
 <div algorithm>
  <dfn abstract-op lt="ReadableStreamDefaultControllerEnqueue"
  id="readable-stream-default-controller-enqueue">ReadableStreamDefaultControllerEnqueue(|controller|,
- |chunk|)</dfn> performs the following steps:
+ |chunk|, |transferList|)</dfn> performs the following steps:

Can `transferList` be optional here? Passing empty list here and there doesn't seem ideal to me.

> @@ -19,6 +19,6 @@ jobs:
         submodules: true
     - uses: actions/setup-node@v1
       with:
-        node-version: 14
+        node-version: 19

You sure you want 19, not 18 LTS?

> @@ -19,6 +19,6 @@ jobs:
         submodules: true
     - uses: actions/setup-node@v1

A good chance to upgrade to setup-node@v3.

>  [Exposed=(Window,Worker,Worklet)]
 interface ReadableStreamDefaultController {
   readonly attribute unrestricted double? desiredSize;
 
   undefined close();
-  undefined enqueue(optional any chunk);
+  undefined enqueue(optional any chunk, optional StructuredSerializeOptions options = { });

```suggestion
  undefined enqueue(optional any chunk, optional StructuredSerializeOptions options = {});
```

`{}` is more conventional.

> @@ -651,8 +651,16 @@ enum ReadableStreamType { "bytes" };
   <p>For an example of how to set up a readable byte stream, including using the different
   controller interface, see [[#example-rbs-push]].
 
-  <p>Setting any value other than "{{ReadableStreamType/bytes}}" or undefined will cause the
-  {{ReadableStream()}} constructor to throw an exception.
+  <p>Can be set to "<dfn enum-value for="ReadableStreamType">owning</dfn>" to signal that the
+  constructed {{ReadableStream}} will own chunks (via transfer or serialization) before enqueuing them.
+  This ensures that enqueued chunks are not mutable by the source.
+  Transferred or serialized chunks may have <dfn>dispose steps</dfn> which are executed if

```suggestion
  Transferred or serialized chunks may have <dfn export>dispose steps</dfn> which are executed if
```

> +  <p>Setting any value other than "{{ReadableStreamType/bytes}}", "{{ReadableStreamType/owning}}"
+  or undefined will cause the {{ReadableStream()}} constructor to throw an exception.

Do we need this? This should be clear from the IDL.

> @@ -1461,7 +1470,7 @@ interface ReadableStreamDefaultController {
   readonly attribute unrestricted double? desiredSize;
 
   undefined close();
-  undefined enqueue(optional any chunk);
+  undefined enqueue(optional any chunk, optional StructuredSerializeOptions options = { });

```suggestion
  undefined enqueue(optional any chunk, optional StructuredSerializeOptions options = {});
```

> @@ -1541,9 +1554,10 @@ the following table:
   previously-enqueued [=chunks=] from the stream, but once those are read, the stream will become
   closed.
 
- <dt><code><var ignore>controller</var>.{{ReadableStreamDefaultController/enqueue()|enqueue}}(<var ignore>chunk</var>)</code>
+ <dt><code><var ignore>controller</var>.{{ReadableStreamDefaultController/enqueue()|enqueue}}(<var ignore>chunk</var>, <var ignore>options</var>)</code>

```suggestion
 <dt><code><var ignore>controller</var>.{{ReadableStreamDefaultController/enqueue()|enqueue}}(|chunk|, |options|)</code>
```

`<var ignore>` is only needed when it's not referenced, and here `chunk` and `options` have their references.

>   <dd>
-  <p>Enqueues the given [=chunk=] <var ignore>chunk</var> in the controlled readable stream.
+  <p>Enqueues the given [=chunk=] <var ignore>chunk</var> in the controlled readable stream,

```suggestion
  <p>Enqueues the given [=chunk=] |chunk| in the controlled readable stream,
```

>   <dd>
-  <p>Enqueues the given [=chunk=] <var ignore>chunk</var> in the controlled readable stream.
+  <p>Enqueues the given [=chunk=] <var ignore>chunk</var> in the controlled readable stream,
+    with <var ignore>options</var>.

```suggestion
    with |options|.
```

>   following steps:
 
  1. Assert: |container| has \[[queue]] and \[[queueTotalSize]] internal slots.
  1. If ! [$IsNonNegativeNumber$](|size|) is false, throw a {{RangeError}} exception.
  1. If |size| is +∞, throw a {{RangeError}} exception.
- 1. [=list/Append=] a new [=value-with-size=] with [=value-with-size/value=] |value| and
+ 1. If |container| has an \[[isOwning]] internal slot whose value is true, perform the following steps:

```suggestion
 1. Let |enqueuedValue| be |value|.
 1. If |container| has an \[[isOwning]] internal slot whose value is true, perform the following steps:
```
`enqueuedValue` initialization is missing for isOwning=false branch.

> @@ -651,8 +651,16 @@ enum ReadableStreamType { "bytes" };
   <p>For an example of how to set up a readable byte stream, including using the different
   controller interface, see [[#example-rbs-push]].
 
-  <p>Setting any value other than "{{ReadableStreamType/bytes}}" or undefined will cause the
-  {{ReadableStream()}} constructor to throw an exception.
+  <p>Can be set to "<dfn enum-value for="ReadableStreamType">owning</dfn>" to signal that the
+  constructed {{ReadableStream}} will own chunks (via transfer or serialization) before enqueuing them.
+  This ensures that enqueued chunks are not mutable by the source.
+  Transferred or serialized chunks may have <dfn>dispose steps</dfn> which are executed if

BTW, chunks should already have dispose steps before transfer/serialization, so this phrase looks a bit off. Can it be:

>Chunks may have dispose steps which are executed when they are owned by the stream via enqueuing and then dequeued without ...

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1271#pullrequestreview-1414608952
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/streams/pull/1271/review/1414608952@github.com>