yutakahirano / fetch-with-streams

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

Introduce FetchReadableByteStream to support bodyUsed. #43

Closed yutakahirano closed 9 years ago

yutakahirano commented 9 years ago

This is an option discussed in #37. Note that the discussion is not finished and this branch is created to see how the wrapper option looks actually.

yutakahirano commented 9 years ago

FetchReadableByteStream has used property which will be set when a lock is acquired. I chose this in order to avoid modifying readers.

domenic commented 9 years ago

In terms of making this more rigorous, I am not sure how to do so without modifying the base streams spec (and implementation).

Specs call AcquireReadableStreamReader(input) to get a reader---they don't go through the public API. So the only things I can think of that would work are:

Also, maybe I am having a hard time reading the spec, but how does this handle new Request(url, { body: normalRbs }) or new Response(normalRbs)? We have to create a wrapper, right?

yutakahirano commented 9 years ago

We can define FetchReadbableByteStream using composition.

class FetchReadableByteStream {
  constructor(...args) {
    this._used = false;
    this._stream = new ReadableByteStream(...args);
  }
  get used() {
    return this._used;
  }
  get locked() {
    return this._stream.locked;
  }
  getReader(...args) {
    this._used = true;
    return this._stream.getReader(...args);
  }
  ...
}

Also, maybe I am having a hard time reading the spec, but how does this handle new Request(url, { body: normalRbs }) or new Response(normalRbs)? We have to create a wrapper, right?

Yes. I think in any case we need wrapper to say something like "body property is of type [Fetch]ReadableByteStream". Otherwise, Request.body and Response.body will be of any type.

domenic commented 9 years ago

We can define FetchReadbableByteStream using composition.

I guess that is true. It means that people cannot modify ReadableByteStream.prototype to add extra methods but we have already moved away from that model with separate ReadableByteStream and ReadableStream.

Maybe this is not so bad.

annevk commented 9 years ago

I guess this can be closed now we decided to just go with ReadableStream.

yutakahirano commented 9 years ago

Yes, thanks.