search

yutakahirano / fetch-with-streams

Fetch API integrated with Streams.
66 stars 8 forks source link

Readable stream and response #18

Closed domenic closed 9 years ago

domenic commented 9 years ago

I am creating this pull request from @yutakahirano's branch simply so I can leave line comments :). Original issue is #17.

domenic commented 9 years ago

In general this looks exactly like how I envisioned streams being used, so I am quite happy. Do you think it makes sense for you as a spec-writer? As an implementer?

Regarding ReadableByteStream vs. ReadableStream, I think we can get away with just using ReadableStream for now while we wait for ReadableByteStream to stabilize. Then we would in the future change these algorithms to specify readInto and use notifyReady, and the only author-facing change would be the addition of a readInto method.

yutakahirano commented 9 years ago

Thank you Domenic, I believe that this is the right way to improve the draft, and I'm happy we agree that. Thanks to you and @tyoshino's comments, it has got better. Regarding implementation, Chrome does different things under the hood, but the draft matches with our mental model.

Regarding ReadableByteStream, I understand. Let's call it ReadableStream for a while.

tyoshino commented 9 years ago

lgtm once the comments are addressed.

yutakahirano commented 9 years ago

Thanks, @tyoshino. Domenic, are you OK with this change?

tyoshino commented 9 years ago

lgtm 0409008

domenic commented 9 years ago

LGTM with minor nits

yutakahirano commented 9 years ago

Thank you for the review!