Re: [whatwg/streams] Add ReadableStreamBYOBReader.readFully() (#1145)

@domenic commented on this pull request.

This seems way too easy! I expected something more like the manual loop you would have to do if writing this in userspace.

If it works, I definitely like this approach. I imagine tests will let us know if it does...

> @@ -1236,7 +1251,10 @@ function ReadableByteStreamControllerCommitPullIntoDescriptor(stream, pullIntoDe
 
   let done = false;
   if (stream._state === 'closed') {
-    assert(pullIntoDescriptor.bytesFilled === 0);
+    assert(pullIntoDescriptor.bytesFilled % pullIntoDescriptor.elementSize === 0);
+    if (!pullIntoDescriptor.readFully) {

Maybe make this if (readFully) { assert1 } else { assert2 }.

Even though your first assert makes sense in both cases, it feels a bit redundant and less clear.

> @@ -1453,7 +1477,7 @@ function ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(contro
   }
 }
 
-function ReadableByteStreamControllerPullInto(controller, view, readIntoRequest) {
+function ReadableByteStreamControllerPullInto(controller, view, readIntoRequest, readFully = false) {

I don't think the defaulting is needed? It only has one (now two) call sites.

> @@ -1550,7 +1575,7 @@ function ReadableByteStreamControllerRespond(controller, bytesWritten) {
 }
 
 function ReadableByteStreamControllerRespondInClosedState(controller, firstDescriptor) {
-  assert(firstDescriptor.bytesFilled === 0);
+  assert(firstDescriptor.bytesFilled % firstDescriptor.elementSize === 0);

Should this be a dual assert like the above?

> @@ -1570,6 +1595,10 @@ function ReadableByteStreamControllerRespondInReadableState(controller, bytesWri
     return;
   }
 
+  if (pullIntoDescriptor.readFully && pullIntoDescriptor.bytesFilled < pullIntoDescriptor.byteLength) {
+    return;

Maybe add a comment here (eventually a spec note) explaining that, IIUC, this is where the magic happens. If we're trying to read fully and haven't filled things up yet, just return, leaving the modified pull-into descriptor in the queue. The next time around we'll pick up right back here, until we're full.

-- 
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/1145#pullrequestreview-710028711