suggestion: deprecate `readableStreamFromReader` and point to better alternatives
Closed this issue · 14 comments
Is your feature request related to a problem? Please describe.
Before I begin my rambling: I created this issue to start a discussion, not sure if this makes any sense at all.
Deno has been continuously moving away from using Deno.reader and (afaik) it is essentially replaced/supplemented in every part of the deno api with a ReadableStream<Uint8Array>. We should continue to push people away from using Deno.reader by deprecating and eventually removing readableStreamFromReader from the std.
To make sure I wasn't completely insane, I did a quick GitHub search and found that almost every use of readableStreamFromReader was for reading files (which supports ReadableStream<Uint8Array>) and reading stdin/stderr (which also supports ReadableStream<Uint8Array>). We should be pushing people away from using these for those use cases.
The only thing that makes me hesitate a little bit is found in the optional options (autoClose, etc)
Describe the solution you'd like
We deprecate and eventually remove readableStreamFromReader
Describe alternatives you've considered
Because of the options, there may be an argument to keep it.
I agree. The GitHub search serves as evidence adequate to support the suggestion, IMO.
Would deprecating readerFromStreamReader also be a good idea for similar reasons?
Don't know how I missed that, nice catch.
I agree that readerFromStreamReader should probably be deprecated/removed but I am a little more cautious because it seems like it would be hard to migrate from this to ReadableStreams.
readableStreamFromReader & its opposite, + the equivalents for writable streams should be deprecated imo
I had a similar thought, but wanted to keep this proposal pretty tame. Good to know I'm not completely out of my mind. Will open a PR for this soon, would like a few more takes from the community.
I'm in favor of deprecation in the future, but I'm not sure we're completely ready to do this.
Do all I/O APIs in Deno namespace provide Web Stream interfaces now?
Deprecated ones like Deno.Buffer do not, but just checked the non-deprecated ones and it looks like they do
I'm in favor of deprecation in the future, but I'm not sure we're completely ready to do this.
Curious on your reasoning for why we aren't ready to do this yet? I feel like all of the apis that would be deprecated in this proposal should really never be used in """modern""" or """correct""" Deno code anyways. If viable alternatives exist, which they do, we should be encouraging their use by deprecating these anti-patterns.
Again, all of this is personal opinion and I am fully open to the fact that I may be missing something.
archive/tar and encoding/binary, for example, seem still depending on Reader/Writer according to #3030 without alternative Web Streams API
Would it make sense to keep readerFromStreamReader around until at least the apis in #1986 have been updated? I'm still frequently using this in Tar and Untar.
@jespertheend
I'd recommend you should use the pinned version, and single export file (like https://deno.land/std@0.201.0/streams/readable_stream_from_reader.ts ).
Does this cause trouble to you?
Yeah, it would be a good workaround for when the day comes. And of course std@0.204.0 still works as well at the moment.
It's just that I have an import map entry for "$std", and importing multiple versions feels a bit tedious/wrong.