miniflare
miniflare copied to clipboard
Impedance mismatch obtaining ReadableStreamBYOB reader from TransformStream readablestream
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
}
}
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. 👍
These are @jasnell questions (& FWIW the proposal I saw used type: "bytes" instead of mode for the TransformStream constructor.
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.
I think this should've been fixed by https://github.com/cloudflare/miniflare/commit/5ab7cb35dc8637f5b1a90c449e33be7e2294fe3d, released in 2.8.0. 👍