denoland / std

The Deno Standard Library
https://jsr.io/@std
MIT License
3.04k stars 606 forks source link

suggestion(streams): deprecate `toArrayBuffer()`, `toBlob()`, `toJson()` and `toText()` #3894

Closed iuioiua closed 9 months ago

iuioiua commented 10 months ago

These functions can essentially be replaced as follows:

  1. toArrayBuffer(stream) to new Response(stream).arrayBuffer()
  2. toBlob(stream) to new Response(stream).toBlob()
  3. toJson(stream) to new Response(stream).json()
  4. toText(stream) to new Response(stream).text()

The main argument against using Response methods was it seems hacky. This argument doesn't justify duplicating functionality that comes for free, already built into the runtime, IMO. If our aim is to complement the Web API, we should remove these APIs.

lino-levan commented 10 months ago

This isn't a fair summary of the situation. Unfortunately, the response hack doesn't work on a lot of things iirc (I'll give an example in a little bit). There's good reason we're not using new Response(whatever).method().

lino-levan commented 10 months ago

One example right away that I found is:

const stringStream = ReadableStream.from(["hello", " deno ", "world"]);
await new Response(stringStream).text(); // throws!

which I feel is a reasonably common case (and is supported by the lib we ported these functions from).

iuioiua commented 10 months ago

That's a misuse of Response rather than a shortcoming. The correct use would just be:

const stringStream = ReadableStream.from(["hello", " deno ", "world"])
  .pipeThrough(new TextEncoderStream());
await new Response(stringStream).text();
lino-levan commented 10 months ago

Response just isn't meant to do this type of work. Here's another example that is really hard to write using new Response(thing).method();:

const mixedStream = ReadableStream.from(["hello", Uint8Array.from([1, 2, 3, 4, 5]), "world", [1, 2, 3, 4, 5]]);
await new Response(mixedStream).blob(); // throws!
nhrones commented 10 months ago

Response just isn't meant to do this type of work. Here's another example that is impossible afaik to write using new Response(thing).method();:

const mixedStream = ReadableStream.from(["hello", Uint8Array.from([1, 2, 3, 4, 5]), "world", [1, 2, 3, 4, 5]]);
await new Response(stringStream).blob(); // throws!

Did you mean

await new Response(mixedStream).blob(); // throws!`

Yes that also throws!

lino-levan commented 10 months ago

Ah sorry, you're right, let me edit that.

iuioiua commented 10 months ago

A stream of mixed types seems like an edge case, and I have yet to see one in testing code, let alone implementation code anywhere. Are you able to find any examples yourself? There are better reasons for adding functionality to the Standard Library than catering to an edge case.

Response just isn't meant to do this type of work.

But it can do this type of work. In this context, I think the Response name should be considered arbitrary, as the methods only consume and convert streams and nothing else. If these methods did do something more than just consumption and conversion that was oriented to, say, serving responses, I'd agree with this point. But there's no technical implication to just using Response methods.

nhrones commented 10 months ago

I guess streaming a KvKey is not a good example?

iuioiua commented 10 months ago

Are you able to elaborate with a snippet?

nhrones commented 10 months ago

Nope!

iuioiua commented 10 months ago

I guess streaming a KvKey is not a good example?

Sorry, I don't understand what you mean.

nhrones commented 10 months ago

I move a lot of KvKeys around in my RPC impls, But I generally serialize first. The example reminded me of a kv-multipart-key.

lino-levan commented 10 months ago

I don't feel super strongly, but I think it provides some value. If you think the cases I've presented do not warrant them existing, I would disagree but I wouldn't be offended if they were deprecated.

kt3k commented 10 months ago

I like to keep these methods.

The main argument against using Response methods was it seems hacky.

I think that's enough good reason to provide these APIs. Newer users can be unaware of those hacky idioms.

iuioiua commented 9 months ago

After some thought, keeping these for newcomers may be the right move. Appreciate the feedback.