Re: [whatwg/streams] Rewrite to use Web IDL, and generally modernize (#1035)

@MattiasBuelens commented on this pull request.

Went through the new spec text. Incredible work! I'm glad to see that almost all of the manual type checks have now been obsoleted by WebIDL. 👏 

I'll go through the reference implementation another time. 😅

>  </div>
 
-<emu-alg>
-  1. If ! IsReadableStreamBYOBReader(*this*) is *false*, return <a>a promise rejected with</a> a *TypeError* exception.
-  1. If *this*.[[ownerReadableStream]] is *undefined*, return <a>a promise rejected with</a> a *TypeError* exception.
-  1. If Type(_view_) is not Object, return <a>a promise rejected with</a> a *TypeError* exception.
-  1. If _view_ does not have a [[ViewedArrayBuffer]] internal slot, return <a>a promise rejected with</a> a *TypeError*
-     exception.
-  1. If ! IsDetachedBuffer(_view_.[[ViewedArrayBuffer]]) is *true*, return <a>a promise rejected with</a> a *TypeError*

This step no longer appears in the new algorithm. Is this intentional?

In the WebIDL conversion steps, it looks like a typed array with a detached buffer gets converted to an empty typed array. From ["getting a reference to a typed array"](https://heycam.github.io/webidl/#ref-for-dfn-get-buffer-source-reference%E2%91%A0) (which I believe gets called from step 5 in ["convert to a typed array"](https://heycam.github.io/webidl/#ref-for-dfn-convert-ecmascript-to-idl-value%E2%91%A4%E2%91%A2)):
> 7. If IsDetachedBuffer(arrayBuffer) is true, then return the empty byte sequence.

Fortunately, there's also this step in `read(view)`, so I guess everything works out correctly in the end?
> 2. If view.[[ByteLength]] is 0, return a promise rejected with a TypeError exception.

---

That said, I'm not entirely sure what [this line in WebIDL](https://heycam.github.io/webidl/#ref-for-dfn-get-buffer-source-reference) entails:
> Attempting to get a reference to or get a copy of the bytes held by a buffer source when the ArrayBuffer has been detached will fail in a language binding-specific manner.

Does WebIDL throw an error when converting, or do we throw an error when getting an empty view?

>  </div>
 
-<emu-alg>
-  1. If ! IsReadableByteStreamController(*this*) is *false*, throw a *TypeError* exception.
-  1. If *this*.[[closeRequested]] is *true*, throw a *TypeError* exception.
-  1. If *this*.[[controlledReadableByteStream]].[[state]] is not `"readable"`, throw a *TypeError* exception.
-  1. If Type(_chunk_) is not Object, throw a *TypeError* exception.
-  1. If _chunk_ does not have a [[ViewedArrayBuffer]] internal slot, throw a *TypeError* exception.
-  1. If ! IsDetachedBuffer(_chunk_.[[ViewedArrayBuffer]]) is *true*, throw a *TypeError* exception.

Assuming my earlier comment is correct: a chunk with a detached buffer will be converted by WebIDL into an empty chunk.

Since we no longer check for a detached buffer ourselves, does this mean that `enqueue(chunk)` will now enqueue an empty chunk and **succeed**, instead of throwing an error? 🤔 

>  
-    <a href="#rbs-controller-close">close</a>()
-    <a href="#rbs-controller-enqueue">enqueue</a>(chunk)
-    <a href="#rbs-controller-error">error</a>(e)
-  }
-</code></pre>
+<xmp class="idl">
+[Exposed=(Window,Worker,Worklet)]
+interface ReadableByteStreamController {
+  readonly attribute ReadableStreamBYOBRequest byobRequest;

From [the steps of the `byobRequest` attribute getter](https://whatpr.org/streams/1035.html#rbs-controller-byob-request): if `this.[[byobRequest]]` is `undefined` but also `this.[[pendingPullIntos]]` **is empty**, then the result should be `undefined`, right? We have [several tests](https://github.com/web-platform-tests/wpt/blob/0ba0c4c07c8d2c23efdcc84dfc9043a3fdccbf19/streams/readable-byte-streams/general.any.js#L698) for that possibility.

Shouldn't the IDL type reflect the possibility of this attribute being `undefined`?

>  
-<emu-alg>
-  1. If ! IsReadableStreamBYOBRequest(*this*) is *false*, throw a *TypeError* exception.
-  1. If *this*.[[associatedReadableByteStreamController]] is *undefined*, throw a *TypeError* exception.
-  1. If Type(_view_) is not Object, throw a *TypeError* exception.
-  1. If _view_ does not have a [[ViewedArrayBuffer]] internal slot, throw a *TypeError* exception.
-  1. If ! IsDetachedBuffer(_view_.[[ViewedArrayBuffer]]) is *true*, throw a *TypeError* exception.

Once again: since we no longer (can) have this check, does that mean that `respondWithNewView(view)` with a detached view will now succeed and behave as if we passed in an empty view?

-- 
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/1035#pullrequestreview-395910612

Received on Saturday, 18 April 2020 15:59:19 UTC