miniflare icon indicating copy to clipboard operation
miniflare copied to clipboard

Impedance mismatch obtaining ReadableStreamBYOB reader from TransformStream readablestream

Open vlovich opened this issue 3 years ago • 3 comments

The runtime currently has non-standard behavior (in the future behind a compat flag) where the ReadableStream part of a TransformStream can return a ReadableStreamBYOBReader. Miniflare however currently throws a TypeError. We have this adapter function currently:

function getReaderHelper(readable: ReadableStream): ReadableStreamBYOBReader {
  try {
    return readable.getReader({ mode: 'byob' })
  } catch (e: any) {
    // instanceOf TypeError returns false, so have to do it this way
    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
    if (e && e.name && e.name === 'TypeError') {
      const reader = readable.getReader()
      const byteReadableStream = new ReadableStream({
        start(controller: ReadableByteStreamController): void {
          async function push(): Promise<void> {
            const read = await reader.read()
            if (read.done) {
              controller.close()
              return
            }
            if (!(read.value instanceof Uint8Array)) {
              throw e
            }
            controller.enqueue(read.value as ArrayBufferView)
            void push()
          }
          void push()
        },
        type: 'bytes',
      })
      return byteReadableStream.getReader({ mode: 'byob' })
    }
    throw e
  }
}

vlovich avatar Mar 29 '22 20:03 vlovich

Hey! 👋 Thanks for raising this. A new TransformStream({ mode: "bytes" }) would've been very helpful when implementing Miniflare. We do something like this for HTTP request/response bodies: https://github.com/cloudflare/miniflare/blob/6c26e320611f5209ba928cb5fba0013b8d4f2a20/packages/core/src/standards/http.ts#L241-L247 I wonder if there's any noticable difference in pulling (my snippet), or pushing (your snippet)? Either way, will get this fixed. 👍

mrbbot avatar Apr 03 '22 16:04 mrbbot

These are @jasnell questions (& FWIW the proposal I saw used type: "bytes" instead of mode for the TransformStream constructor.

vlovich avatar Apr 07 '22 03:04 vlovich

The TransformStream constructor accepts readableType and writableType options but they are explicitly required to be undefined right now. That is, there is no standard way for a TransformStream to operate in BYOB mode. We have introduced IdentityTransformStream as an alternative to cover that case. It is, and will remain, exactly like what our non-standard TransformStream is today.

jasnell avatar Apr 07 '22 05:04 jasnell

I think this should've been fixed by https://github.com/cloudflare/miniflare/commit/5ab7cb35dc8637f5b1a90c449e33be7e2294fe3d, released in 2.8.0. 👍

mrbbot avatar Sep 13 '22 15:09 mrbbot