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

ReadableStream.prototype.text(), .blob(), and .bytes() #1311

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link

@jasnell jasnell commented Apr 9, 2024

Adds utility methods to akin to the Body mixin for fully reading the contents of a ReadableStream as either text, a Blob, or a Uint8Array.

This is a first pass at addressing #1019 ....

In this, I add only the text(), bytes(), and blob() methods. text() returns a promise for UTF-8 decoded text, bytes() returns a promise for a Uint8Array, and blob() returns a promise for a Blob. It is modeled heavily on the fetch spec Body mixin definitions with a few tweaks.

Refs: #1019


  • At least two implementers are interested (and none opposed):
    • Cloudflare Workers
    • Node.js
    • ...
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno: …
    • Node.js: …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Adds utility methods to akin to the Body mixin for fully reading the
contents of a ReadableStream as either text, a Blob, or a Uint8Array.
otherwise.
1. Assert: either |signal| is undefined, or |signal| [=implements=] {{AbortSignal}}.
1. Let |decode as Uint8Array| be an algorithm that takes a [=byte sequence=] |bytes| and runs the steps:
1. Let |arrayBuffer| be a new {{ArrayBuffer}} whose contents are |bytes|.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a faithful copy of the text in fetch, but fetch has a bug which is copied here. It doesn't necessarily need to be fixed here, just gotta make sure it gets fixed here as well when fixed upstream.

Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just random comments at the moment, not a full review.

1. Let |successSteps| given a [=byte sequence=] |bytes| be to run |processBody| given |bytes|.
1. Let |errorSteps| optionally given an exception |exception| be to run |processBodyError| given |exception|.
1. Let |reader| be the result of getting a reader for |stream|. If that threw an exception, then run |errorSteps| with that exception and return.
1. [=read all bytes|Read all bytes=] from |reader|, given |successSteps| and |errorSteps|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Bikeshed can compensate for different capitalisation automatically.


It performs the following steps:

1. Assert: |stream| [=implements=] {{ReadableStream}}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could have a PrepareToConsume abstract op to encapsulate these common steps?


dictionary ConsumeBytesOptions {
AbortSignal signal;
USVString type = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This type is not super clear about what type it means, maybe blobType? Especially if we want to put more stream related options, because we already have type in UnderlyingSource.
  2. Blob uses DOMString instead of USVString: https://w3c.github.io/FileAPI/#ref-for-dfn-BPtype
  3. Should we have a separate dictionary without type for other non-blob methods?

@@ -2661,6 +2688,99 @@ create them does not matter.
1. Return « |branch1|, |branch2| ».
</div>

<div algorithm>
To <dfn lt="fully read">fully read</dfn> a {{ReadableStream}} |stream|, given an algorithm |processBody|, and an algorithm |processBodyError|, run these steps. |processBody| must be an algorithm accepting a [=byte sequence=]. |processBodyError| must be an algorithm optionally accepting an exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this lt? Same for other <dfn>s.

</div>

<div algorithm>
The <dfn lt="consume fully with abort">consume fully with abort</dfn> algorithm, given a {{ReadableStream}} |stream|, an optional {{AbortSignal}} |signal|, and an algorithm that takes a [=byte sequence=] and returns a JavaScript or throws an exception |convertBytesToJSValue|, runs these steps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"consume fully" or "fully consume"? We already have "fully read", so maybe consistency?

And also "an algorithm |convertBytesToJSValue| that ..."


1. Let |promise| be [=a new promise=].
1. Let |errorSteps| given |error| to be [=reject=] |promise| with |error|.
1. Let |successSteps| given a [=byte sequence=] |data| be to [=resolve=] |promise| with the result of running |convertBytesToJSValue| with |data|. If that threw an exception, then run |errorSteps| with that exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you want to name them as processBody and processBodyError as that's what the parameters of "fully read" are? Or maybe "fully read" parameters should be success/errorSteps instead.

1. If |stream|.[=ReadableStream/[[state]]=] is "`readable`", [=set/append=] the following action action to |actions|:
1. return ! [$ReadableStreamCancel$](|stream|, |error|).
1. Otherwise, return [=a promise resolved with=] undefined.
1. [=Shutdown with an action=] consisting of [=getting a promise to wait for all=] of the actions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pipeTo specific, maybe we can simply call ReadableStreamCancel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants