Re: [whatwg/streams] Various fixes for readable byte streams (#1123)

@domenic approved this pull request.

Overall LGTM, although @ricea's review would be good. One potential note:

> .enqueue(chunk) and .respondWithNewView(newView) now check that the given view's buffer is actually transferable. Previously, we only checked whether the buffer is not yet detached, but this is insufficient: a WebAssembly.Memory's buffer is never transferable. We also make sure to not transfer the given buffer until after we've checked all other preconditions, so the buffer is still intact if these methods were to throw an error.

I'm wondering if we should restructure things to naturally let TransferArrayBuffer throw (and perhaps that thrown exception would get turned into a promise rejection in some cases). This only works if we can transfer after all precondition checks, but before any other state mutations. But at a glance that appears to be how you've factored things?

I guess that's kind of a subtle hidden requirement on us as spec maintainers though, which is not great...

The reason I say this is because I suspect in implementations it's much easier to just transfer than it is to check for transferability. So they might want to implement things that way anyway.



-- 
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/1123#pullrequestreview-661425686