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

Introduce FetchReadableByteStream to support bodyUsed. #43

Closed
wants to merge 2 commits into from

Conversation

yutakahirano
Copy link
Owner

No description provided.

@yutakahirano
Copy link
Owner Author

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
Copy link
Owner Author

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

@domenic
Copy link
Contributor

domenic commented Jun 10, 2015

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:

  • Modify base streams spec AcquireReadableStreamReader. This basically adds the used flag to streams, which I have been arguing against. We could make it only spec-exposed I guess, but that kind of cheating implies to me that we are doing something wrong.
  • Go through the public API? Then we would define FetchReadableByteStream as a subclass which is identical to ReadableStream except that it overrides getReader to set the used flag. Although this would mean you could bypass setting the used flag with ReadableStream.prototype.getReader.call(fetchRbs).

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
Copy link
Owner Author

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
Copy link
Contributor

domenic commented Jun 11, 2015

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
Copy link

annevk commented Jul 27, 2015

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

@yutakahirano
Copy link
Owner Author

Yes, thanks.

@yutakahirano yutakahirano deleted the fetch-readable-byte-stream branch July 29, 2015 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants