Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggestion: deprecate readableStreamFromReader and point to better alternatives #3018

Closed
lino-levan opened this issue Dec 19, 2022 · 14 comments

Comments

@lino-levan
Copy link
Contributor

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.

@iuioiua
Copy link
Collaborator

iuioiua commented Dec 19, 2022

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?

@lino-levan
Copy link
Contributor Author

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.

@crowlKats
Copy link
Member

readableStreamFromReader & its opposite, + the equivalents for writable streams should be deprecated imo

@lino-levan
Copy link
Contributor Author

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.

@iuioiua
Copy link
Collaborator

iuioiua commented Dec 20, 2022

@kt3k, WDYT?

@kt3k
Copy link
Member

kt3k commented Dec 21, 2022

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?

@crowlKats
Copy link
Member

Deprecated ones like Deno.Buffer do not, but just checked the non-deprecated ones and it looks like they do

@lino-levan
Copy link
Contributor Author

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.

@kt3k
Copy link
Member

kt3k commented Dec 21, 2022

archive/tar and encoding/binary, for example, seem still depending on Reader/Writer according to #3030 without alternative Web Streams API

@crowlKats
Copy link
Member

There also is #1986

@kt3k
Copy link
Member

kt3k commented Sep 15, 2023

closed by #3640

@kt3k kt3k closed this as completed Sep 15, 2023
@jespertheend
Copy link
Contributor

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.

@kt3k
Copy link
Member

kt3k commented Oct 23, 2023

@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?

@jespertheend
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants