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

Remove pure JS (non native) io functions from Deno namespace #9795

Closed
13 tasks done
lucacasonato opened this issue Mar 15, 2021 · 17 comments
Closed
13 tasks done

Remove pure JS (non native) io functions from Deno namespace #9795

lucacasonato opened this issue Mar 15, 2021 · 17 comments
Assignees
Labels
breaking change a change or feature that breaks existing semantics feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS
Milestone

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Mar 15, 2021

I propose we remove the following APIs from the Deno namespace, and move them to std:

Types:

  • Deno.Reader
  • Deno.Writer
  • Deno.ReaderSync
  • Deno.WriterSync
  • Deno.Closer

Functions:

Timeline

I suggest we mark all the APIs we want to remove with @deprecated, and add a deno lint diagnostic for when these APIs are used. In Deno 2.0 we can then remove these APIs.

Reasoning

Code which does not actually require native syscalls should not use Deno namespace APIs as this makes the code incompatible with browsers, even though there is no need for that to be the case. An example of this: https://deno.land/std@0.90.0/io/streams.ts. This uses Deno APIs, even though no native calls are preformed, making this code unusable in a web browser for no good reason.

@lucacasonato lucacasonato added public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed) breaking change a change or feature that breaks existing semantics maybe 2.0 labels Mar 15, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Mar 15, 2021

Why move the types? They are referred to by other built ins that are returned, are they not?

@kitsonk
Copy link
Contributor

kitsonk commented Mar 15, 2021

Buffer is also mentioned in #9588, we should maybe close that?

@lucacasonato
Copy link
Member Author

Why move the types?

Same reason as to move the functions. Code using Reader / Writer etc does not work when lib is dom instead of deno.ns. IMO if it doesnt need to be a binding (or a type for those bindings) because it can work just fine in the browser, it should not be part of the Deno namespace.

They are referred to by other built ins that are returned, are they not?

The builtins never explicitly return or take a reader/writer after these changes as far as i'm aware. They only return types that implement Reader or Writer. Instead of using implement in the d.ts, we can just specify the read / write methods no the interfaces directly. This would make them implicitly implement Reader / Writer (just like interfaces work in Go).

@timreichen
Copy link
Contributor

@lucacasonato in addition to that: would you be open to rename
Deno.iter -> Deno.iterator
Deno.iterSync -> Deno.iteratorSync
for aligning with the js name like in Symbol.iterator?

@lucacasonato
Copy link
Member Author

No. They are both in the process of being deprecated and will be removed. No reason to rename.

@lucacasonato lucacasonato added feat new feature (which has been agreed to/accepted) and removed suggestion suggestions for new features (yet to be agreed) labels Apr 26, 2021
@ry
Copy link
Member

ry commented Jul 5, 2021

I'm in agreement. It's very painful to let web streams (ReadableStream/WritableStream) win over go streams (Reader/Writer). Go streams is plainly more general and vastly simpler. But we must have web streams because of fetch. And having two different and incompatible stream abstractions is worse than having one shitty one.

One of the original intentions of Deno was to correct the abysmal streams implementation in Node. I think Go provides a very elegant and complete solution with a beautiful library of combinations.

@MierenManz
Copy link

We just have to hope that there will be a webstream2 at some point in the future that is based on go streams :/

@nayeemrmn
Copy link
Collaborator

Just to clarify, this issue was about a purely symbolic refactor to remove the explicit type definitions of Reader and Writer in favour of just defining the read() and write() methods on their implementers. This is logical because there are no stable built-ins that accept these interfaces, only ones that return them. A one sided protocol so to speak. Since they do have consumers in std, it would be more appropriate to have the definitions there.

Why @ry mentions above is the result of more recent discussion to remove even the concrete read() and write() methods in favour of web stream APIs. We should continue in #11018 for that.

@kt3k
Copy link
Member

kt3k commented Dec 22, 2022

We're removing the usages of Deno.Reader, Deno.Writer, ... from std in denoland/std#3030

@iuioiua
Copy link
Contributor

iuioiua commented Aug 25, 2023

Types:

  • Deno.Reader
  • Deno.Writer
  • Deno.ReaderSync
  • Deno.WriterSync
  • Deno.Closer

Perhaps, 2.0 is a good time to deprecate these interfaces. A counterargument is fast streams implementations are still in progress, and I wouldn't expect people to be happy with the deprecation unless the performance of the alternative was on par or better.

@kt3k
Copy link
Member

kt3k commented Aug 25, 2023

What to do about Deno.Seeker and Deno.SeekerSync?

@kt3k
Copy link
Member

kt3k commented Sep 13, 2023

There seem a few concerning points to this move:

  • There seem no alternative way to support synchronous I/O using ReadableStream based APIs.
  • The support of seek behavior will be awkward though it's still possible to perform with mixed usage of ReadableStream and existing seek() method

@kt3k
Copy link
Member

kt3k commented Sep 14, 2023

sync behavior might be implemented as FileSystemSyncAccessHandle in the future

seek is available in FileSystemWritableFileStream, but I couldn't find a way to seek during reading in Web APIs

@iuioiua
Copy link
Contributor

iuioiua commented Dec 28, 2023

Update: std/log no longer depends on Deno.Writer. See: denoland/std#4021

@iuioiua
Copy link
Contributor

iuioiua commented Jan 29, 2024

What to do about Deno.Seeker and Deno.SeekerSync?

What do we think about this? Best to come to a decision before Deno v2. Related: Deno.seek() and Deno.seekSync() were deprecated in Deno 1.40.

@bartlomieju
Copy link
Member

All of these APIs are no longer available as of #25213 and Deno 2.0.0-rc.0.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 10, 2024

We'll be also moving Deno.Seeker[Sync] into @std/io/types of the Standard Library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change a change or feature that breaks existing semantics feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS
Projects
None yet
Development

No branches or pull requests

9 participants