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

@MattiasBuelens requested changes on this pull request.



>  
+ 1. Let |transferList| be |options|["transfer"].
+ 1. If |transferList| is not [=list/is empty|empty=] and [=this=].[=ReadableStreamDefaultController/[[isOwning]]=] is false

```suggestion
 1. If |transferList| is not [=list/is empty|empty=] and [=this=].[=ReadableStreamDefaultController/[[isOwning]]=] is false,
```

> @@ -2287,10 +2306,10 @@ create them does not matter.
       1. Otherwise, set |chunk2| to |cloneResult|.\[[Value]].
      1. If |canceled1| is false, perform !
         [$ReadableStreamDefaultControllerEnqueue$](|branch1|.[=ReadableStream/[[controller]]=],
-        |chunk1|).
+        |chunk1|, undefined).

`ReadableStreamDefaultControllerEnqueue` takes a list as third argument, and does not handle `undefined`. (It would fail on the "Assert: *transferList* is empty" check.)

You need to pass an explicit empty list here:
```suggestion
        |chunk1|, « »).
```

>       1. If |canceled2| is false, perform !
         [$ReadableStreamDefaultControllerEnqueue$](|branch2|.[=ReadableStream/[[controller]]=],
-        |chunk2|).
+        |chunk2|, undefined).

```suggestion
        |chunk2|, « »).
```

>  
+ 1. Assert: |transferList| is empty or |controller|.[=ReadableStreamDefaultController/[[isOwning]]=] is true.

```suggestion
 1. Assert: |transferList| [=list/is empty=] or |controller|.[=ReadableStreamDefaultController/[[isOwning]]=] is true.
```

> +       1. Set |internalChunk| to [$StructuredTransferOrClone$](|chunk|, |transferList|).
+       1. If |internalChunk| is an abrupt completion,
+          1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |internalChunk|.\[[Value]]).
+          1. Return |internalChunk|.

`internalChunk` starts out as a regular JavaScript value (with value `chunk`), but then changes into a completion record. We then use this record as-is later on in `ReadableStreamFulfillReadRequest`, which is wrong.

We need to manually unwrap the completion:
```suggestion
       1. Let |result| be [$StructuredTransferOrClone$](|chunk|, |transferList|).
       1. If |result| is an abrupt completion,
          1. Perform ! [$ReadableStreamDefaultControllerError$](|controller|, |result|.\[[Value]]).
          1. Return |result|.
       1. Set |internalChunk| to |result|.\[[Value]].
```

> @@ -5884,7 +5913,7 @@ The following abstract operations support the implementaiton of the
  1. If ! [$ReadableStreamDefaultControllerCanCloseOrEnqueue$](|readableController|) is false, throw
     a {{TypeError}} exception.
  1. Let |enqueueResult| be [$ReadableStreamDefaultControllerEnqueue$](|readableController|,
-    |chunk|).
+    |chunk|, undefined).

```suggestion
    |chunk|, « »).
```

> + 1. If |container| has an \[[isOwning]] internal slot whose value is true, perform the following steps:
+ 1. If |container| has a \[[isOwning]] internal slot whose value is true, perform the following steps:

Accidentally duplicated line. 😅
```suggestion
 1. If |container| has an \[[isOwning]] internal slot whose value is true, perform the following steps:
```

> +    1. Set |enqueuedValue| to [$StructuredTransferOrClone$](|value|, |transferList|).
+    1. If |enqueuedValue| is an abrupt completion, return |enqueuedValue|.

Use `?` to unwrap the completion.
```suggestion
    1. Set |enqueuedValue| to ? [$StructuredTransferOrClone$](|value|, |transferList|).
```

> @@ -6447,7 +6484,7 @@ abstract operations are used to implement these "cross-realm transforms".
   1. Let |value| be ! [$Get$](|data|, "`value`").
   1. Assert: [$Type$](|type|) is String.
   1. If |type| is "`chunk`",
-   1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](|controller|, |value|).
+   1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](|controller|, |value|, undefined).

```suggestion
   1. Perform ! [$ReadableStreamDefaultControllerEnqueue$](|controller|, |value|, « »).
```

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

```suggestion
    const transferList = options ? options.transfer : [];
```
Otherwise `if (transferList.length)` will throw. 😉

> @@ -37,9 +39,23 @@ exports.PeekQueueValue = container => {
   return pair.value;
 };
 
+const closingStepsSymbol = Symbol('closing-steps');

This symbol isn't exposed anywhere, so I suppose we can't really write tests for this? 😕

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

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