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

Proposal: ReadableStreamBodyReader #1238

Closed
jasnell opened this issue Jul 5, 2022 · 12 comments
Closed

Proposal: ReadableStreamBodyReader #1238

jasnell opened this issue Jul 5, 2022 · 12 comments

Comments

@jasnell
Copy link

jasnell commented Jul 5, 2022

Consuming a ReadableStream as either an ArrayBuffer, Blob, FormData, JSON, or text is a common use case. We can see this in Body mixin from fetch and the methods implemented by Request and Response, as well as in utilities such as Node.js' custom "stream consumers" documented here: https://nodejs.org/dist/latest-v18.x/docs/api/webstreams.html#utility-consumers

The pattern is so common, it would be helpful to be able to standardize the pattern with a new type of standard reader type... Specifically something like:

interface ReadableStreamBodyReader {
  constructor(ReadableStream stream);

  readonly attribute boolean bodyUsed;
  [NewObject] Promise<ArrayBuffer> arrayBuffer();
  [NewObject] Promise<Blob> blob();
  [NewObject] Promise<FormData> formData();
  [NewObject] Promise<any> json();
  [NewObject] Promise<USVString> text();

  undefined releaseLock();
}
ReadableStreamBodyReader includes ReadableStreamGenericReader;

Use example:

const readable = getReadableStreamSomehow();
const reader = readable.getReader({ mode: 'body' });
await reader.arrayBuffer();
// or
await reader.text();
// or
await reader.json();
// or
await reader.formData();
// or
await reader.blob();

Note that the ReadableStreamGenericReader could but does not just include the Body mixin. This is because the Body mixin includes the body getter that really wouldn't make sense in this case. We could use it, however, if that is the preference.

The key benefit of adding this is that it would standardize what is becoming a common pattern.

@lucacasonato
Copy link
Member

lucacasonato commented Jul 5, 2022

@jasnell How would reader.formData() work? It requires access to a content-type to be able to parse form data.

@jasnell
Copy link
Author

jasnell commented Jul 5, 2022

Couple of options there:

  1. Calling formData() can assume that the content is multipart/form-data, rejecting the promise only if it cannot be successfully parsed as such, or...
  2. The formData() method here could be augmented to accept an init parameter specifying a type. This would make it a departure from the Body mixin definition but achieves the goal.

@lucacasonato
Copy link
Member

Sorry, I wasn't clear. body.formData() requires the ;boundary=<boundary> parameter of the content type to parse, so parsing it without a content type is not an option. I think requiring folks use new Response(stream, { headers }).formData() is probably fine. (ie don't include form data in this proposal)

@jasnell
Copy link
Author

jasnell commented Jul 5, 2022

ah right, forgot about that boundary parameter on the content-type. In that case, we either do as you suggest and limit it to new Response(stream, { headers }).formData() or allow for an init argument on reader.formData() to specify the content-type. As one may imagine implementing new Response(stream, { headers }).formData() using this reader, the latter could make sense.

@lucacasonato
Copy link
Member

As one may imagine implementing new Response(stream, { headers }).formData() using this reader

You mean as an engine, or as a user? Because new Response(stream).json() etc is already possible.

I think the crux of this proposal is if it makes sense to add another stream reader type, considering one can already do the proposed behaviour with new Response(stream).json() etc.

Succinctness alone is not a compelling argument in my opinion, because the new reader mode is more verbose than using new Response:

> "readable.getReader({ mode: 'body' })".length
36
> "new Response(readable)".length
22

@MattiasBuelens
Copy link
Collaborator

Making this a proper reader can also have weird consequences. What should happen if you call releaseLock() while arrayBuffer() is still pending, and then you acquire a new reader?

const stream = new ReadableStream({ type: "bytes" });
const reader1 = stream.getReader({ mode: "body" });
reader1.arrayBuffer().catch(() => {});
// ...some time passes
reader1.releaseLock();
const reader2 = stream.getReader();
await reader2.read(); // what does this return?

Will reader2 see the full contents of stream? Or will reader1 have already consumed some bytes, such that reader2 only sees the remaining bytes?

It's also a bit weird conceptually: the Body mixin is defined on top of ReadableStream, but now you'd be able to consume a ReadableStream as a Body too? It's unclear which of the two is the "base primitive". 😛

We already proposed the idea of adding something like ReadableStream.prototype.arrayBuffer() in #1019. I think that's a better starting point, and then response.arrayBuffer() could become a shorthand for response.body.arrayBuffer(). 🙂

@jasnell
Copy link
Author

jasnell commented Jul 5, 2022

What should happen if you call releaseLock() while arrayBuffer() is still pending, and then you acquire a new reader?

I think in that case we'd have no choice but to error the stream if releaseLock() is called while one of these operations is pending.

@jasnell
Copy link
Author

jasnell commented Jul 6, 2022

Succinctness alone is not a compelling argument in my opinion, because the new reader mode is more verbose than using new Response:

No, definitely not the only argument. The goal is to standardize the pattern even in cases where Response/Request are not used. If all I want is to be able to consume a ReadableStream as a concatenated text string, I shouldn't have to create a Response object to do so. The ReadableStream.prototype.arrayBuffer() option would be a good alternative to the body reader proposal, although ReadableStream.prototype.blob() might be better as it gives more opportunity for internal optimizations.

@jimmywarting
Copy link

How would reader.formData() work? It requires access to a content-type to be able to parse form data.

the stream could also just read the first line and acquire the boundary from reading just some initial bytes...

@jasnell
Copy link
Author

jasnell commented Mar 23, 2024

What are folks' thoughts about advancing this proposal forward? I think for the FormData issue we could actually just omit formData() from this proposal.

@ricea
Copy link
Collaborator

ricea commented Mar 25, 2024

My preference is for #1019.

@jasnell
Copy link
Author

jasnell commented Mar 25, 2024

Let's close this in favor of the other proposal

@jasnell jasnell closed this as not planned Won't fix, can't repro, duplicate, stale Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants