feat(std/io): add toReader & toWriter

[marcosc90]

Currently, we can do the following with fetch res.body

  const res = await fetch("http://example.com/file.bin");
  const file = await Deno.open("/tmp/file.bin", { create: true, write: true });

  await Deno.copy(res.body, file);
  file.close();

If we make Response (#5787) spec-compliant the above code will stop working. I think it should still work.

[nayeemrmn]

What about WritableStream in place of Writer, and all of the other APIs that take a Reader? Clearly there should be a more general approach to bridging these APIs.

[marcosc90]

What about WritableStream in place of Writer, and all of the other APIs that take a Reader? Clearly there should be a more general approach to bridging these APIs.

Make sense. We can have the following methods.

toReader(src: ReadableStream): Reader;
toWriter(dst: WritableStream): Writer;

[ry]

Readable and Writeable stream are unfortunate complexities...

[marcosc90]

Readable and Writeable stream are unfortunate complexities...

Yeah, I know. But we should definitely support it.

I made some changes to the return types, toWriter returns Writer & Closer to avoid exporting a new type.
We can discuss the API if someone has strong opinions about this.

[bartlomieju]

I have only one remark. @ry please review

[bartlomieju]

I guess those utils are useful but exposing them as Deno.toWriter() and Deno.toReader() doesn't seem to be a good idea

[marcosc90]

My initial version was only for internal use but thought users will need a way to convert between Streams & Reader/Writer. Don't mind making them internal-only again.

[bartlomieju]

Great tests

[lucacasonato]

Shouldn't all of the ReadableStream and WriteableStream be ReadableStream<Uint8Array> and WriteableStream<Uint8Array>?

[marcosc90]

I'm not used to setting types for streams and I missed it. Should be ReadableStream<Uint8Array | string> and WriteableStream<Uint8Array>.

[nayeemrmn]

We should add the following to the API:

function fromStreamReader(
  streamReader: ReadableStreamDefaultReader<Uint8Array>,
): Deno.Reader {
  const buffer = new Deno.Buffer(new Uint8Array(4096));
  return {
    async read(b: Uint8Array): Promise<number | null> {
      if (buffer.empty()) {
        const { done, value } = await streamReader.read();
        if (done) {
          return null;
        }
        await Deno.writeAll(buffer, value!);
      }
      return buffer.read(b);
    },
  };
}

function fromStreamWriter(
  streamWriter: WritableStreamDefaultWriter<Uint8Array>,
): Deno.Writer {
  return {
    async write(b: Uint8Array): Promise<number> {
      await streamWriter.write(b);
      return b.length;
    },
  };
}

The original example will become:

const res = await fetch("http://example.com/file.bin");
const file = await Deno.open("/tmp/file.bin", { create: true, write: true });

await Deno.copy(Deno.fromStreamReader(res.body.getReader()), file);
file.close();

[nayeemrmn]

Again, what is the intuition behind just copy() supporting these and not every other IO function? This shouldn't be included in favour of just the conversion utilities.

[marcosc90]

Wanted to do progressive changes instead of a massive PR (Initially it was meant to be an issue to discuss the API, but since I had some code that I was testing submitted a PR instead).

I'm good with only having utilities to convert from/to and leave Deno.copy as is.

Now If we do only utilities, should they be in the core or in std?

[nayeemrmn]

We have Deno.iter() so I think these are candidates to be included in the CLI.

[ry]

It's certainly handy for copy to take ReadableStream/WritableStream but in the interest of being conservative in our API changes, I think toReader and toWriter would solve the problem for now. Let's do the most minimal thing and expand on it later.

That is, I agree with @nayeemrmn, please undo this change.

[marcosc90]

I'm working on it and now I'm wondering whether we want to make the API take a ReadableStream or a ReadableStreamDefaultReader / ReadableStreamBYOBReader, the same goes for WritableStream

[nayeemrmn]

The latter is the logical and minimal choice. It's only one more method call to use on a *ableStream.

cc @kitsonk

[kitsonk]

I believe the reader/writer interfaces are what we want to use (the latter).

[ry]

New API additions should be unstable. Can you move these to lib.deno.unstable.d.ts ?

I'm not sure about the names in particular...

[marcosc90]

Will do!. Regarding the names, I'm open to suggestions.

@nayeemrmn suggested:

• fromStreamReader
• fromStreamWriter

[ry]

Looks great @marcosc90 - thanks for this patch - I just want to be careful about expanding our API. I think the most minimal thing is to add toWriter and toReader to the unstable API and not make any changes to the copy function.

[marcosc90]

Looks great @marcosc90 - thanks for this patch - I just want to be careful about expanding our API. I think the most minimal thing is to add toWriter and toReader to the unstable API and not make any changes to the copy function.

Sounds good. I'll work on the modifications and update in the next couple of days.

[marcosc90]

This is ready for review. Regarding the names, I don't care much, so whatever you prefer works for me.

toReader
toWriter

or

fromStreamReader
fromStreamWriter

Or any other alternative.

[ry]

@marcosc90 Maybe an even more minimal way to address this it to move toReader and toWriter into std. I understand that this would be less ergonomic - but it would allow us to experiment and iterate on these APIs before committing to adding them to the runtime. What do you think?

[marcosc90]

I think it is a good idea, I asked about adding it to std here #5789 (comment)

Should I place them in std/io/util.ts?

[nayeemrmn]

How about std/io/streams.ts for all things WHATWG streams related? Though I don't think there is much to iterate on, honestly.

[ry]

std/io/streams.ts seems reasonable to me

[nayeemrmn]

LGTM

[ry]

LGTM - thank you for seeing this one through @marcosc90