Replace byte stream with ReadableStream. #200
Conversation
| members as the <code title=concept-ReadableStream>ReadableStream</code> object has today. | ||
|
|
||
| <p>A <span title=concept-body>body</span> has an associated | ||
| <dfn title=concept-body-transmitted>transmitted</dfn> (an integer). Unless stated it is 0. |
There was a problem hiding this comment.
"Unless stated otherwise it is 0". Same below.
|
This looks really great @yutakahirano. Apart from the above nits, I think whenever we allocate a I would also greatly appreciate it if @domenic and @wanderview could have a look at this PR (search for |
|
Regarding OOM handling, I would create a separate PR. |
yutakahirano
force-pushed
the
body-stream-refactoring
branch
from
8bf5455 to
5b90955
Compare
|
I squashed the commits. |
|
@domenic said he would take a look, but I think he's been in meetings all week. I suspect he can review this on Monday. Would that be alright with you? |
|
Yes, thanks! |
| <dfn title=concept-body-transmitted>transmitted</dfn> which is an integer and initially 0, | ||
| <dfn title=concept-body-length>length</dfn> which is an integer and initially 0, and | ||
| <dfn title=concept-body-error-flag>error flag</dfn> which is initially unset. | ||
| <p>A <dfn title=concept-body>body</dfn> consists of a |
There was a problem hiding this comment.
I think this would be slightly clearer like:
A body consists of:
- A stream (a ReadableStream object), initially null (?).
- A transmitted (an integer), initially 0.
- A total bytes (an integer), initially 0.
And maybe s/transmitted/transmitted bytes/g?
There was a problem hiding this comment.
Done.
Body's stream cannot be null and has no initial value. We always specify its stream explicitly when we create a body.
yutakahirano
force-pushed
the
body-stream-refactoring
branch
2 times, most recently
from
d5fdeab to
b924be8
Compare
| <p><span title=concept-enqueue-ReadableStream>Enqueue</span> a <code>Uint8Array</code> | ||
| object wrapping an <code>ArrayBuffer</code> containing <var>bytes</var> to <var>stream</var>. | ||
|
|
||
| <p class="note no-backref">This operation will not throw an exception. |
There was a problem hiding this comment.
Since this is false, we should not add this note.
yutakahirano
force-pushed
the
body-stream-refactoring
branch
3 times, most recently
from
561421d to
ea70963
Compare
|
Wit those nits fixed I think this is ready to land. I'd appreciate if you could then fix XMLHttpRequest so that everything still makes some sense. If there's other things we'll just fix them as they come. Getting more eyes on all this Streams language is useful. |
|
@yutakahirano I'm happy to merge this by the way. If you want to do it, please see the README for instructions. You're probably already familiar with it from Streams, we basically prefer a clean history with single commits for a change. |
|
Unless someone tells me otherwise I plan to land this on Monday. |
yutakahirano
force-pushed
the
body-stream-refactoring
branch
2 times, most recently
from
eeb79df to
9ea3c2f
Compare
This change replaces byte streams used in request and response with ReadableStream. This change also unifies some properties and operations which was defined separately in Request and Response classes. The change is based on https://github.com/yutakahirano/fetch-with-streams/blob/master/body-readable-stream.md. Depending specs need to be changed as well.
yutakahirano
force-pushed
the
body-stream-refactoring
branch
from
9ea3c2f to
35db942
Compare
|
AI:
|
|
Thank you very much, reviewers! |
|
What does AI mean? Something similar to TODO? Thank your for working on this. It's greatly appreciated. |
|
Yes, I meant TODO. |
This change replaces byte streams used in request and response with ReadableStream. This change
also unifies some properties and operations which was defined separately in Request and Response
classes.
The change is based on
https://github.com/yutakahirano/fetch-with-streams/blob/master/body-readable-stream.md.
Depending specs need to be changed as well.