only use ponyfilled ReadableStream when necessary #151
Conversation
pgoldberg
force-pushed
the
pgoldberg/dontAlwaysPolyfillReadableStream
branch
from
660a58b to
e2a3a06
Compare
pgoldberg
force-pushed
the
pgoldberg/dontAlwaysPolyfillReadableStream
branch
2 times, most recently
from
f4ff8d2 to
b9f1dde
Compare
| start: controller => { | ||
| export function blobToReadableStream( | ||
| blobPromise: Promise<Blob>, | ||
| ): ReadableStream<Uint8Array> | PonyfilledReadableStream<Uint8Array> { |
There was a problem hiding this comment.
nit: Is it a bit strange for PonyfilledReadableStream<T> to be exposed as a return type of this function? Ideally the polyfill usage of this function is an implementation detail that doesn't affect its calling API contract.
Assuming this was added since PonyfilledReadableStream<T> isn't a subset of ReadableStream<T>?
There was a problem hiding this comment.
I do think it's strange, but unfortunately it is necessary.
Type 'ReadableStream<Uint8Array> | ReadableStream<...>' is not assignable to type 'ReadableStream<Uint8Array>'.
Type 'import("/Volumes/git/conjure-typescript-runtime/node_modules/web-streams-polyfill/dist/types/polyfill").ReadableStream<Uint8Array>' is not assignable to type 'ReadableStream<Uint8Array>'.
Types of property 'getReader' are incompatible.
Type '{ ({ mode }: { mode: "byob"; }): ReadableStreamBYOBReader_2; (): ReadableStreamDefaultReader_2<Uint8Array>; }' is not assignable to type '() => ReadableStreamDefaultReader<Uint8Array>'.
There was a problem hiding this comment.
Gotcha. Thanks for copying out the compile error.
Although I dislike as assertions, I'm a bit conflicted on whether an as ReadableStream<Uint8Array> type assertion in this function might be the best option. The | PonyfilledReadableStream<Uint8Array> addition to the return type here might mean downstream consumers have to cast that portion away later. If a cast somewhere in the call stack is necessary, it might be best to do it here where it's introduced?
What do you think?
There was a problem hiding this comment.
Filling in some offline discussion here:
The ponyfill typing and the native ReadableStream type don't have sufficient overlap to cast, so we have to cast as unknown first. Currently we cast as any here, downstream of blobToReadableStream:
So this shouldn't affect downstream consumers regardless, but changing anyway to prevent any future change to that cast unintentionally affecting downstream consumers. Also, @gluxon pointed out we probably should have needed to change the return type here:
Not sure why that wasn't causing a compilation error
pgoldberg
force-pushed
the
pgoldberg/dontAlwaysPolyfillReadableStream
branch
from
b9f1dde to
311dad8
Compare
|
Force push was just a rebase after #152 merged |
There was a problem hiding this comment.
Looks good on my side! (Pending some discussion on the return type changing.) Let's get a look from @walkerburgin or @styu before merging too.
Before this PR
web-streams-polyfilldoesn't include any checks to see if the browser it's running in has native streams. This causes conflicts between the native streams and the polyfilled ones.After this PR
==COMMIT_MSG==
We now use the ponyfilled
ReadableStreamfromweb-streams-polyfillonly if the browser doesn't haveReadableStream.==COMMIT_MSG==
Possible downsides?
This already existed, but
web-streams-polyfillwill still get loaded even if it's unused.