-
Notifications
You must be signed in to change notification settings - Fork 367
Add a textStream() method to the Body mixin #1862
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
base: main
Are you sure you want to change the base?
Conversation
|
||
<li><p>Let <var>stream</var> be <a>this</a>'s <a for=Body>body</a>'s <a for=body>stream</a>. | ||
|
||
<li><p>Let <var>decoder</var> be a new {{TextDecoderStream}} object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the next step would need improving if there's interest in this method.
The constructor steps for TextDecoderStream
aren't implicitly run by just saying "new object", but what I'm after is an UTF-8 decoder with the default settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MattiasBuelens or @ricea might have thoughts, but ideally we don't use public API for internal algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably expose an algorithm to create a TextDecoderStream
from other specifications, like we do for Streams.
|
||
<li><p>Let <var>decoder</var> be a new {{TextDecoderStream}} object. | ||
|
||
<li><p>Return the result of piping <var>stream</var> through <var>decoder</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here "pipe through" would need to link to something. I'm not sure if there are other APIs that internally do something similar that I could copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use the pipe through algorithm exposed for other specifications. [=piping through=]
should work. 😉
|
||
<li><p>Let <var>stream</var> be <a>this</a>'s <a for=Body>body</a>'s <a for=body>stream</a>. | ||
|
||
<li><p>Let <var>decoder</var> be a new {{TextDecoderStream}} object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MattiasBuelens or @ricea might have thoughts, but ideally we don't use public API for internal algorithms.
</div> | ||
|
||
<div algorithm> | ||
<p>The <dfn method for=Body><code>textStream()</code></dfn> method steps are:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the null case it'd be nicer to return an empty stream, similar to what text()
does.
Another thing that's not entirely clear to me is whether piping ends up making the underlying stream unusable so that you can't invoke text()
after textStream()
for instance. (That's a property we want.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize what text()
does if this.body
is null. I agree an empty stream would be better, code that doesn't consider this case would likely just work.
I thought this should behave like when you try to use both blob()
and bytes()
, or indeed call one of those twice. That throws a TypeError in the first step of https://fetch.spec.whatwg.org/#concept-body-consume-body.
Since it's not possible to call text()
twice, should it really be possible to call text()
after textStream()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, that should be easy to achieve, even though the current spec text doesn't make clear what happens.
Fixes #1861.
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff