- From: Adam Rice <notifications@github.com>
- Date: Thu, 08 Feb 2018 06:43:52 -0800
- To: whatwg/streams <streams@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/streams/pull/875/review/94980679@github.com>
ricea commented on this pull request.
> @@ -502,6 +470,91 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
1. Otherwise, throw a *RangeError* exception.
</emu-alg>
+<h4 id="underlying-source-api">Underlying Source API</h4>
It appears that this is now normative. In which case, it needs to correspond exactly with the algorithm steps or the standard will be unimplementable.
> @@ -502,6 +470,91 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
1. Otherwise, throw a *RangeError* exception.
</emu-alg>
+<h4 id="underlying-source-api">Underlying Source API</h4>
+
+The {{ReadableStream()}} constructor accepts as its first argument a JavaScript object representing the <a>underlying
+source</a>. Such objects may contain any of the following properties:
+
+<dl>
+ <dt><dfn method for="underlying source">start(<var>controller</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called immediately upon creation of the {{ReadableStream}}.</p>
Perhaps we should day "during creation" to make the timing unambiguous?
> @@ -502,6 +470,91 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
1. Otherwise, throw a *RangeError* exception.
</emu-alg>
+<h4 id="underlying-source-api">Underlying Source API</h4>
+
+The {{ReadableStream()}} constructor accepts as its first argument a JavaScript object representing the <a>underlying
+source</a>. Such objects may contain any of the following properties:
+
+<dl>
+ <dt><dfn method for="underlying source">start(<var>controller</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called immediately upon creation of the {{ReadableStream}}.</p>
+
+ <p>Typically this is used adapt a <a>push source</a> by setting up relevant event listeners, as in the example of
+ [[#example-rs-push-no-backpressure]], or to acquire access to a <a>pull source</a>, as in [[#example-rs-pull]].</p>
+
+ <p>If this setup process is asynchronous, it can return a promise to signal success or failure; a rejected promise
Or a thenable.
Probably we should mention that pull() will not be called until start() is done.
> +<dl>
+ <dt><dfn method for="underlying source">start(<var>controller</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called immediately upon creation of the {{ReadableStream}}.</p>
+
+ <p>Typically this is used adapt a <a>push source</a> by setting up relevant event listeners, as in the example of
+ [[#example-rs-push-no-backpressure]], or to acquire access to a <a>pull source</a>, as in [[#example-rs-pull]].</p>
+
+ <p>If this setup process is asynchronous, it can return a promise to signal success or failure; a rejected promise
+ will error the stream. Any thrown exceptions will be re-thrown by the {{ReadableStream()}} constructor.</p>
+ </dd>
+
+ <dt><dfn method for="underlying source">pull(<var>controller</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called whenever the stream's <a>internal queue</a> of chunks becomes not full, i.e. whenever
+ the queue's <a lt="desired size to fill a stream's internal queue">desired size</a> becomes positive. It will be
Nitpick: it will only be called repeatedly as long as it enqueues at least one chunk. A no-op `pull()` will not be called repeatedly.
> + called repeatedly until the queue reaches its <a>high water mark</a> (i.e. until the <a lt="desired size to fill a
+ stream's internal queue">desired size</a> becomes non-positive).</p>
+
+ <p>For <a>push sources</a>, this can be used to resume a paused flow, as in [[#example-rs-push-backpressure]]. For
+ <a>pull sources</a>, it is used to acquire new <a>chunks</a> to enqueue into the stream, as in
+ [[#example-rs-pull]].</p>
+
+ <p>If the function returns a promise, then it will not be called again until that promise fulfills. (If the
+ promise rejects, the stream will become errored.) This is mainly used in the case of pull sources, where the
+ promise returned represents the process of acquiring a new chunk. Throwing an exception is treated the same as
+ returning a rejected promise.</p>
+ </dd>
+
+ <dt><dfn method for="underlying source">cancel(<var>reason</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called whenever the <a>consumer</a> signals they are no longer interested in the stream, via
Also `pipeTo()` if `preventCancel` is not set. I don't know whether it's better to mention that or not.
> @@ -2757,12 +2810,19 @@ throws>SetUpReadableByteStreamControllerFromUnderlyingSource ( <var>stream</var>
const bytes = new Uint8Array(writer.desiredSize);
window.crypto.getRandomValues(bytes);
- await writer.write(bytes);
+ // Purposefully don't await; awaiting writer.ready is enough.
+ writer.write(bytes);
How about adding `.catch(() => {})` to avoid uncaught rejections?
> }
}
writeRandomBytesForever(myWritableStream).catch(e => console.error("Something broke", e));
</code></pre>
+
+ Note how we don't <code>await</code> the promise returned by {{WritableStreamDefaultWriter/write()}}. Doing so would
+ prevent us from executing further writes until the previous one succeeded, which defeats the point of the stream's
+ <a>internal queue</a>. Instead, we <code>await</code> the {{WritableStreamDefaultWriter/ready}} promise. That promise
+ will fulfill when the <a lt="desired size to fill a stream's internal queue">desired size</a> becomes positive, which
+ might be before the write succeeds (especially in cases with a larger <a>high water mark</a>).
Sadly this comment is not correct, because `write()` always fills the queue in one go. See longer explanation at https://github.com/whatwg/streams/issues/874#issuecomment-364120545. Maybe just leave it out?
> 1. Let _sizeAlgorithm_ be ? MakeSizeAlgorithmFromSizeFunction(_size_).
+ 1. Let _highWaterMark_ be ? GetV(_readableStrategy_, `"highWaterMark"`).
Because MakeSizeAlgorithmFromSizeFunction() will throw if size is not a function, placing this GetV() after that operation changes behaviour.
> </div>
<emu-alg>
1. Perform ! InitializeReadableStream(*this*).
1. Let _type_ be ? GetV(_underlyingSource_, `"type"`).
1. Let _typeString_ be ? ToString(_type_).
+ 1. Let _size_ be ? GetV(_readableStrategy_, `"size"`).
Maybe move these up so that existing behaviour doesn't change?
> + {{WritableStreamDefaultWriter/close()|writer.close()}}, that they are done writing <a>chunks</a> to the stream, and
+ subsequently all queued-up writes have successfully completed.</p>
+
+ <p>This function can perform any actions necessary to finalize or flush writes to the <a>underlying sink</a>, and
+ release access to any held resources.</p>
+
+ <p>If the shutdown process is asynchronous, the function can return a promise to signal success or failure; the
+ result will be communicated via the return value of the called
+ {{WritableStreamDefaultWriter/close()|writer.close()}} method. Additionally, a rejected promise will error the
+ stream, instead of letting it close successfully. Throwing an exception is treated the same as returning a rejected
+ promise.</p>
+ </dd>
+
+ <dt><dfn method for="underlying sink">abort(<var>reason</var>)</dfn></dt>
+ <dd>
+ <p>A function that is called after the <a>producer</a> signals, via {{WritableStream/abort()|stream.abort()}} or
Maybe mention `pipeTo()`?
> + <p>A function that is called after the <a>producer</a> signals, via {{WritableStream/abort()|stream.abort()}} or
+ {{WritableStreamDefaultWriter/abort()|writer.abort()}}, that they wish to abruptly close the stream and put it in an
+ errored state, discarding any queued-up writes. It takes as its argument the same value as was passed to those
+ methods by the producer.</p>
+
+ <p>This function can clean up any held resources, much like {{underlying sink/close()}}, but perhaps with some
+ custom handling.</p>
+
+ <p>If the shutdown process is asynchronous, the function can return a promise to signal success or failure; the
+ result will be communicated via the return value of the called <code>abort()</code> method. Throwing an exception is
+ treated the same as returning a rejected promise. Regardless, the stream will be errored with a new {{TypeError}}
+ indicating that it was aborted.</p>
+ </dd>
+</dl>
+
+The <code>controller</code> argument passed to {{underlying sink/start()}} and {{underlying sink/write()}} is an
`controller.error()` should be rarely used, so I don't want to draw undue attention to it. I think this text is fine as-is.
> + this by sometimes enqueuing zero chunks.</p>
+
+ <p>If the process of transforming is asynchronous, this function can return a promise to signal success or failure
+ of the transformation. A rejected promise will error both the readable and writable sides of the transform
+ stream.</p>
+
+ <p>If no {{transformer/transform()}} is supplied, the identity transform is used, which enqueues chunks unchanged
+ from the writable side to the readable side.</p>
+ </dd>
+
+ <dt><dfn method for="transformer">flush(<var>controller</var>)</dfn></dt>
+ <dd>
+ <p>A function called after all <a>chunks</a> written to the <a>writable side</a> have been transformed by
+ successfully passing through {{transformer/transform()}}, and the writable side is about to be closed.</p>
+
+ <p>Typically this is used to enqueue suffix chunks to the <a>readable side</a>, before that too becomes closed. An
Do you think we should mention that there's no need to explicitly call `controller.close()` inside `flush()`? I made this mistake in my early text encoder stream implementation.
> @@ -4501,7 +4643,42 @@ aoid="TransformStreamDefaultSourcePullAlgorithm" nothrow>TransformStreamDefaultS
<h2 id="other-stuff">Other Stream APIs and Operations</h2>
-<h3 id="blqs-class" interface lt="ByteLengthQueuingStrategy">Class <code>ByteLengthQueuingStrategy</code></h3>
+<h3 id="qs">Queuing Strategies</h3>
+
+<h4 id="qs-api">The Queuing Strategy API</h4>
+
+The {{ReadableStream()}}, {{WritableStream()}}, and {{TransformStream()}} constructors all accept at least one argument
+representing an appropriate <a>queuing strategy</a> for the stream being created. Such objects contain the following
+properties:
+
+<dl>
+ <dt><dfn method for="queuing strategy">size(<var>chunk</var>)</dfn> (non-byte streams only)</dt>
+ <dd>
+ <p>A function that computes and returns the size of the given <a>chunk</a> value.</p>
+
+ <p>The result is used to determine <a>backpressure</a>, manifesting via
I find this sentence hard to parse, but I don't have a concrete suggestion for how to improve it at the moment.
> @@ -4895,6 +5072,21 @@ writable stream:
.catch(e => console.error("Something went wrong!", e));
</code></pre>
+<div class="note" id="note-web-socket-wrapping-examples">
+ This specific style of wrapping a web socket interprets web socket messages directly as <a>chunks</a>.
+ This can be convenient in some cases, for example when <a>piping</a> to a <a>writable stream</a> or <a>transform
How about saying "This can be a convenient abstraction, ..." instead.
The phrasing "in some cases" sounds a bit too negative to my ears.
> @@ -4895,6 +5072,21 @@ writable stream:
.catch(e => console.error("Something went wrong!", e));
</code></pre>
+<div class="note" id="note-web-socket-wrapping-examples">
+ This specific style of wrapping a web socket interprets web socket messages directly as <a>chunks</a>.
+ This can be convenient in some cases, for example when <a>piping</a> to a <a>writable stream</a> or <a>transform
+ stream</a> for which each web socket message makes sense as a chunk to consume or transform.
+
+ However, often when people talk about "adding streams support to web sockets", they are hoping instead for the ability
How about "for a new capability" instead of "for the ability"? To emphasise the distinction between wrapping an existing API, as we are doing here, and increasing the capabilities of the platform, as we'd like to do.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/875#pullrequestreview-94980679
Received on Thursday, 8 February 2018 14:44:17 UTC