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

Make FS and net APIs work well with WHATWG streams #13614

Closed
lucacasonato opened this issue Feb 7, 2022 · 5 comments · Fixed by #13615
Closed

Make FS and net APIs work well with WHATWG streams #13614

lucacasonato opened this issue Feb 7, 2022 · 5 comments · Fixed by #13615
Labels
public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate suggestion suggestions for new features (yet to be agreed)

Comments

@lucacasonato
Copy link
Member

Currently our FS and net APIs use "Go style" streams (Reader / Writer). While these kinds of streams are very fast and simple, they are not very well compatible with fetch and all our newfangled CompressionStream and TextEncoderStream utilities.

Currently std/streams has readableStreamFromReader and writableStreamFromWriter functions to convert from Writer to WritableStream and from Reader to ReadableStream. While these are convenient, they are not as convenient as not having to use them at all.

To solve this I am proposing adding a readable and writable property to all of our internal implementations of Reader and Writer. readable would return a ReadableStream<Uint8Array> and writable would return a WritableStream<Uint8Array>.

This makes Deno.File really convenient to use for use with the native HTTP server or fetch for example:

const file = await Deno.open("/etc/passwd");
const resp = await fetch("https://example.com", {
  method: "POST",
  body: file.readable
});
await resp.body.pipeTo(Deno.stdout.writable);

This could land independently of how #11018. These two APIs can co-exist - it doesn't need to be one or the other.

@lucacasonato lucacasonato added public API related to "Deno" namespace in JS suggestion suggestions for new features (yet to be agreed) runtime Relates to code in the runtime crate labels Feb 7, 2022
@crowlKats
Copy link
Member

crowlKats commented Feb 7, 2022

From a quick look, this seems like a good solution. I'd say that this is "blocked" on whatwg/streams#1145, and writable byte stream (there isnt a concrete issue about it, but such a concept is mentioned in various issues)

@lucacasonato
Copy link
Member Author

I am not quite sure why the former would be needed. As for writable byte streams: how would they be different to regular write streams? There is no point in "bring your own buffer" for writers, as you always bring your own buffer there (you give the writable stream something to write).

@crowlKats
Copy link
Member

i agree for the former thats why "blocked" with quotes. regarding the latter: byte ≠ byob, there is a difference.
iirc in one of the mentions of writablebytestream, write would have a similar interface to our Writer.write: write(buf: ArrayBufferView): Promise<number> (or something like that, i cant remember, but it would return the amount of bytes written. take this with a grain of salt, i cant fully remember and has been a while since i read that). else, the writable we would have on Writer would always be like our current writeAll.

@lucacasonato
Copy link
Member Author

else, the writable we would have on Writer would always be like our current writeAll.

Yup, this is what my current thinking was. I think adding support for these "byte" streams would be a backwards compatible change (or would it not?).

@crowlKats
Copy link
Member

crowlKats commented Feb 7, 2022

Well, from my findings there is no spec suggestion, so i have no idea, but i assume its gonna be a similar deal to readablestream, so then yea, it would be backwards compatible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants