Implement teeing, plus a bunch of stage-setting tweaks #311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@tyoshino hoping to merge this tomorrow, would be great if you had review time today :). They're not too bad, I promise. |
|
Sorry, Domenic. I was OOO on Friday. I've done reviewing till the spec side change of b326a6e. I think you can go ahead committing it. I'll leave comments and we can discuss them later. |
For the tee algorithm being drafted in #302, it's important to have an abstract operation CloseReadableStream that does all the things that ReadableStreamController.prototype.close does. So, we factor that out. The old CloseReadableStream is renamed to "FinishClosingReadableStream".
Similarly to the previous commit, we want this for defining the tee algorithm (and potentially others, e.g. from other specifications).
Following the established pattern, public API methods should throw if preconditions are violated, whereas abstract operations should assert that they are not.
In particular, https://github.com/yutakahirano/fetch-with-streams is an obvious client for all of the ones noted.
Again, needed for formalizing the tee algorithm from #302.
Instead of creating a closure in the constructor, storing it on the stream, and calling this@[[error]] all the time, we can instead abstract out an ErrorWritableStream(stream, e) abstract operation and call that when appropriate. Then, the WritableStream error functions just delegate to this abstract operation. The big upside of this is that, if the error function ends up not being used by the underlying sink (e.g. the underlying sink relies on returning rejected promises instead), we can immediately garbage-collect it, instead of carrying it around for the lifetime of the stream. While doing so, make the editorial move to ES-spec-style closures, with internal slots, instead of using the "closing over" technique. It turns out that when you try to extend the "closing over" language to more complicated situations (as we will do for teeing), it falls over, becoming very confusing. The ES-spec version of closures is obtuse, but works. Finally, we now put the definitions of closures underneath the place that creates them (in this case the constructor), instead of creating a whole new CreateWritableStreamErrorFunction abstract operation, defined some distance from the relevant function. This will serve us well for tee and pipe.
|
Thanks for the review! Updating and merging. Links to outstanding discussions: |
domenic
added a commit
to domenic/fetch-with-streams
that referenced
this pull request
Apr 6, 2015
In whatwg/streams#311, abstract operations were factored out to allow other specs (like this one) to operate on readable streams more directly. We now use them.
|
Wow, all the links you've kindly listed are revoked as they were placed on this issue... Sad. |
|
I would be in favor of not exposing |
This was referenced May 31, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR supercedes #302. It is based on the same algorithm, but fully formalized and moved into the spec.
To get it formalized, I had to do a lot of cleanup around other abstract operations---mostly factoring out abstract operations from places that used to be accessible only through the public API. This should help create a better version of yutakahirano/fetch-with-streams#28, for example. Each of the commits should be pretty simple to review individually.
The only real decisions that I want to highlight are:
rs.tee()method does not do any cloning; the TeeReadableStream abstract operation takes a second parameter that lets you decide whether to do structured cloning or not. Presumably for uses that will share across threads, like much of fetch, we'll need cloning.rs.tee()at all? I mostly exposed it because otherwise my unit testing setup was not able to test it :P. But I can work around that, certainly. Maybe it is best to leave it off for now? I am a bit afraid that having non-cloningrs.tee()available would be a footgun for fetch body streams, given that many of the things you do with fetch body streams involve shuffling them off to another thread. Or do we assume that if you're going to do such things, you'll use the wholeresponseobject, and if you doresponse.body.tee(), you know what you're getting in to? That also seems plausible.Branch snapshot at https://streams.spec.whatwg.org/branch-snapshots/tee-again/; sections of note: