Standard text for TransformStream #811

ricea · 2017-09-26T15:03:46Z

Add text to the standard for TransformStream.

Preview | Diff

ricea · 2017-09-26T15:04:21Z

This is very incomplete. I created this PR to permit early commenting.

ricea · 2017-09-26T15:09:51Z

https://streams.spec.whatwg.org/branch-snapshots/transform-stream-standard-text/

ricea · 2017-10-12T09:24:44Z

This is now complete and ready for review.

If possible, I would like both @tyoshino and @domenic to review the formatted text. Almost all the changes are in the transform streams model section and the Transform Streams section. All of the issues I've fixed in my own checks have been visible in the formatted version.

I think we need one volunteer to pick through all the new markup for errors, and another volunteer to double-check that the steps in the text match the steps in the reference implementation. I think it's best to coordinate so we don't duplicate this grungy work.

domenic · 2017-10-12T16:28:17Z

I can certainly volunteer for markup review for now.

domenic · 2017-10-12T22:02:54Z

I did a first pass focused on non-normative text. Of note, I tried to eliminate talking about "output" and "input", given the confusion in #174. Instead I introduced "writable side" and "readable side" and tried to use those as much as possible.

I think we should also add some nice examples to the appendix. Ideas:

Wrapping a TextEncoder or TextDecoder
A small suite of simple transforms such as uppercase , double the output, etc.
Wrapping web cypto?

I'll try to do more review tomorrow.

ricea · 2017-10-13T04:23:16Z

index.bs

+    writable.close();
+  </code></pre>
+
+  Another use of identity transform streams is to add additional buffering to a <a>pipe</a>. In this example we add


Is it possible to have a separate id for the buffering example without disrupting the readability of this section too much?

Sure, we can just make it another <div>, not a big deal I think.

ricea · 2017-10-13T04:25:51Z

index.bs

-      append a suffix to the output data. If this process is asynchronous, it can return a promise
-      to signal success or failure. On success, the {{readable}} side will be closed.
+    <li><p><code>start(controller)</code> is called immediately, and is typically used to enqueue prefix <a>chunks</a>
+      that appear in the <a>readable side</a> but doesn't depend on any writes to the <a>writable side</a>. If this


"... don't depend on ..."

"appear" is awkward here. Maybe "will be read from"?

ricea · 2017-10-13T13:55:53Z

98157cd lgtm. Explicitly defining "readable side" and "writable side" solves a lot of the wording problems I struggled with.

tyoshino · 2017-10-17T12:51:03Z

index.bs

+<h3 id="ts-intro">Using Transform Streams</h3>
+
+<div class="example" id="example-basic-pipe-through">
+  The natural way to use a transform stream is place it in a <a lt="piping">pipe</a> between a <a>readable stream</a>


place -> placing?

We could use "by placing it" or "to place it". I think I prefer "to place it".

tyoshino · 2017-10-17T12:56:09Z

index.bs

+    writable.close();
+  </code></pre>
+
+  Another use of identity transform streams is to add additional buffering to a <a>pipe</a>. In this example we add


tyoshino · 2017-10-17T13:06:25Z

index.bs

+  1. If _readableType_ is not *undefined*, throw a *RangeError* exception.
+  1. Let _writableType_ be ? GetV(_transformer_, `"writableType"`).
+  1. If _writableType_ is not *undefined*, throw a *RangeError* exception.
+  1. Let _controller_ be  ? Construct(`<a idl>TransformStreamDefaultController</a>`, « *this* »).


extra space between be and ?

tyoshino · 2017-10-17T13:07:58Z

index.bs

+  1. Perform ! CreateDataProperty(_readableStrategy_, `"highWaterMark"`, _highWaterMark_).
+  1. Set *this*.[[readable]] to ? Construct(`<a idl>ReadableStream</a>`, « _source_, _readableStrategy_ »).
+  1. Let _sink_ be ! Construct(`<a idl>TransformStreamDefaultSink</a>`, « *this*, _startPromise_ »).
+  1. Set *this*.[[writable]] to ? Construct(`<a idl>WritableStream</a>`, « _sink_, _writableStrategy_ »).


missing writableStrategy creation

It comes directly from the constructor arguments. I can add a note to explain the asymmetry between readableStrategy and writableStrategy.

Oops, sorry. I should've checked the arguments list.

Yeah, it might be helpful, thought it's just because I'm not catching up with the changes. Is fd17cf0 the only background of the asymmetry?

Yes, and issue #777 which discusses it.

ricea · 2017-10-17T14:11:06Z

I made some fixes in 4d2d14e which is on this branch but inexplicably missing from this PR. If it doesn't show up by tomorrow I will do a rebase to force it.

domenic · 2017-10-20T20:08:49Z

OK, going to work on checking that the steps match up now.

domenic

Reviewed everything. Pretty happy.

I left one comment about comments being translated to notes, and I think it applies more generally. I'd be happy to add more notes to the algorithms in the same place you added comments in the .js file. I wish we'd done that with the other streams, but since we have that here, it'd be nice.

domenic · 2017-10-20T20:22:06Z

index.bs

+  1. Perform ! WritableStreamDefaultControllerErrorIfNeeded(_stream_.[[writable]].[[writableStreamController]], _e_).
+  1. If _stream_.[[readable]].[[state]] is `"readable"`, perform !
+     ReadableStreamDefaultControllerError(_stream_.[[readable]].[[readableStreamController]], _e_).
+  1. If _stream_.[[backpressure]] is *true*, perform ! TransformStreamSetBackpressure(_stream_, *false*).


This has a comment in the source that seems like it might be useful to include in the standard as a note. However it references things I'm not sure about, like write()/pull()/enqueue(). Are those on the transform stream source/sink? Maybe it can be rephrased to capture the important points without the details; not sure.

domenic · 2017-10-20T20:39:45Z

index.bs

+<h5 id="ts-default-source-cancel" method for="TransformStreamDefaultSource">cancel(<var>reason</var>)</h5>
+
+<emu-alg>
+  1. Perform ! TransformStreamError(*this*.[[ownerTransformStream]], _reason_).


Just noticing this... is this correct behavior? I'd have thought it'd be more like terminate than error.

It actually does behave like terminate, because the readable side is already closed when cancel() is called. I added a note to make this clearer.

Is there a reason to call TransformStreamError instead of TransformStreamDefaultControllerTerminate? I guess layering, hmm... seems to argue for factoring out a TransformStreamTerminate, if I am understanding correctly.

We'd end up with TypeError('TransformStream terminated') as the storedError in the writable. This seems less desirable to me than retaining whatever was passed to cancel().

However... maybe factoring out the "bottom half" of TransformStreamDefaultControllerTerminate and TransformStreamError is the way to go. I will try it.

I made factored out a new abstract op named "TransformStreamErrorWritableAndUnblockWrite". PTAL.

ricea · 2017-10-27T14:57:40Z

I reviewed the comments in the reference implementation to see if any would be useful as notes in the standard. I added a couple more notes. To be honest, I feel that inline notes can make the algorithm harder to follow. It would be nice to have a separate non-normative "algorithm overview" section where we explain how it works. But we don't have anything like that for ReadableStream and WritableStream (and it might be prohibitively complicated to have an up-to-date explanation for those).

Anyway, I believe that implementers will find out how it works in the process of implementing it. Ordinary web developers don't need to know why it works. The mental model of "transform is called when there is no backpressure" is super-easy to understand and the subtleties involved in delivering that are not their problem.

domenic · 2017-10-31T23:47:03Z

Made some tweaks, ready to merge when you are!

ricea · 2017-11-01T10:01:01Z

1a26588 lgtm

Add algorithm steps for TransformStreamDefaultController constructor, desiredSize getter, enqueue() method, TransformStreamDefaultControllerEnqueue abstract operation. For TransformStreamDefaultSink, the constructor, start(), write(), abort() and close() methods.

Also fix a linking error in TransformStreamDefaultControllerEnqueue. Now the only algorithms remaining to be done are those that are expected to change.

TransformStreamDefaultSinkInvokeTransform and TransformStreamDefaultSinkTransform.

- Linkify some things that were not linkified. - Add extension points for readableType / writableType. - Note what the readable / writable accessors are for to make linking to - them more helpful. - Correctly indicate the throwyness of TransformStreamDefaultControllerEnqueue - Add notes for the TSDefaultController abstract ops that intended to be used from other specifications. - Use GetV to access object properties.

Previously the TransformStream constructor used readableStrategy["foo"] syntax. Use Set() instead.

Use CreateDataProperty() instead of Set(). Make the method for constructing the object more concrete.

Also - markup writableStrategy as a <var> in the TransformStream constructor header - add a missing "to" in example-basic-pipe-through - remove an excess space from the TransformStream constructor algorithm steps

All add a comment to the reference implementation to note the same thing.

Explain the reason for calling TransformStreamSetBackpressure there.

Factor out TransformStreamErrorWritableAndUnblockWrite from TransformStreamError and TransformStreamDefaultControllerTerminate. Use the new operation in TransformStreamDefaultSource cancel() to make the semantics clearer.

Explain why the constructor initialises the [[backpressure]] slot to *undefined* and note that implementations can do something else. Also add a note that TransformStreamError can be used even when an error has already occurred. Also change TransformStreamDefaultControllerEnqueue to call TransformStreamErrorWritableAndUnblockWrite rather than TransformStreamError when ReadableStreamDefaultControllerEnqueue throws, since we know that the readable side will already be errored in that case.

It should have been _enqueueValue_.[[Value]]. Also correct `_chunk_` to `_e_` in the arguments to TransformStreamDefaultControllerError and eliminate a pointless local from that operation.

Move the initialisations of [[transformStreamController]] and [[backpressure]] to right before they are needed. This should make the reasons for how they are initialised more obvious.

ricea · 2017-11-01T10:03:30Z

Rebased.

ricea · 2017-11-02T01:04:46Z

I will go ahead and land this since @tyoshino is busy. We can always fix any issues he finds later.

TejasQ · 2017-11-02T08:42:45Z

index.bs

+
+    enqueue(chunk)
+    error(reason)
+    terminate()


I find it interesting that we have cancel(), terminate(), and close() on different types of streams.

Yes, they all have different semantics. So based on the principle of "different things should look different", we use different names for them. See #774 for the discussion where we added terminate(). The difference between cancel() and close() is mentioned in the FAQ: https://github.com/whatwg/streams/blob/master/FAQ.md#why-dont-errors-that-occur-while-cancelling-put-the-readable-stream-in-an-error-state

That's awesome. Definitely a lesson in there for me. Thanks, @ricea.

TejasQ · 2017-11-02T08:50:04Z

I'm building a demo of this as soon as support lands in Canary/any nightly of any browser.

ricea added do not merge yet Pull request must not be merged per rationale in comment transform streams labels Sep 26, 2017

ricea added the editorial Changes that do not affect how the standard is understood label Sep 27, 2017

domenic mentioned this pull request Sep 28, 2017

Make it easy to create streams with no underlying source/sink/transformer objects, from specs #813

Closed

ricea force-pushed the transform-stream-standard-text branch 2 times, most recently from cb88cab to 9b31882 Compare October 5, 2017 15:16

ricea force-pushed the transform-stream-standard-text branch 2 times, most recently from 40f9ef2 to e8453fc Compare October 12, 2017 08:46

ricea removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 12, 2017

ricea requested review from domenic and tyoshino October 12, 2017 09:18

ricea changed the title ~~WIP: Standard text for TransformStream~~ Standard text for TransformStream Oct 12, 2017

ricea commented Oct 13, 2017

View reviewed changes

ricea force-pushed the transform-stream-standard-text branch from 5ebb666 to d054b80 Compare October 16, 2017 11:07

tyoshino reviewed Oct 17, 2017

View reviewed changes

domenic approved these changes Oct 20, 2017

View reviewed changes

domenic assigned ricea Nov 1, 2017

ricea and others added 23 commits November 1, 2017 19:02

Add algorithm steps for TransformStream{Error,SetBackpressure}

0a74a47

Add algorithms from TransformStreamDefaultSource

6174197

Also fix a linking error in TransformStreamDefaultControllerEnqueue. Now the only algorithms remaining to be done are those that are expected to change.

Move terminate() method and add TSDCTerminate op

b84bff2

Add algorithm for TransformStream constructor

4a26b32

Add algorithm steps for DefaultController error() and terminate()

e2e250d

Add algorithm text for DefaultController Error and Terminate

6412779

Add Transform Stream Default Sink Abstract Operations

564c0f1

TransformStreamDefaultSinkInvokeTransform and TransformStreamDefaultSinkTransform.

Fix string quoting and use Set()

f1f02ad

Previously the TransformStream constructor used readableStrategy["foo"] syntax. Use Set() instead.

Tweaks to non-algorithm prose: model, etc.

0ef972a

Reword note about transformer start() method

eae0793

Revise algorithm for creating _readableStrategy_

b1f51b6

Use CreateDataProperty() instead of Set(). Make the method for constructing the object more concrete.

Explain the asymmetry between readable and WritableStragy in a note

4d78c63

Also - markup writableStrategy as a <var> in the TransformStream constructor header - add a missing "to" in example-basic-pipe-through - remove an excess space from the TransformStream constructor algorithm steps

More input/output -> writable side/readable side

5794de1

Nits

9db49a0

Note TSDefaultSource cancel() leaves the readable side closed

702141d

All add a comment to the reference implementation to note the same thing.

Add a note to TransformStreamError

9d3e4ab

Explain the reason for calling TransformStreamSetBackpressure there.

Factor out TransformStreamErrorWritableAndUnblockWrite

66804ba

Factor out TransformStreamErrorWritableAndUnblockWrite from TransformStreamError and TransformStreamDefaultControllerTerminate. Use the new operation in TransformStreamDefaultSource cancel() to make the semantics clearer.

Replace uninitialsed _e_ in TransformStreamDefaultControllerEnqueue

73e8f38

It should have been _enqueueValue_.[[Value]]. Also correct `_chunk_` to `_e_` in the arguments to TransformStreamDefaultControllerError and eliminate a pointless local from that operation.

Change order of initialisations in TransformStream constructor

bad61e6

Move the initialisations of [[transformStreamController]] and [[backpressure]] to right before they are needed. This should make the reasons for how they are initialised more obvious.

Minor tweaks

cfd14a0

ricea force-pushed the transform-stream-standard-text branch from 1a26588 to cfd14a0 Compare November 1, 2017 10:03

ricea merged commit 7457dd9 into master Nov 2, 2017

ricea deleted the transform-stream-standard-text branch November 2, 2017 01:08

TejasQ reviewed Nov 2, 2017

View reviewed changes

Standard text for TransformStream #811

Standard text for TransformStream #811

Conversation

ricea commented Sep 26, 2017 • edited by pr-preview bot Loading

ricea commented Sep 26, 2017

ricea commented Sep 26, 2017

ricea commented Oct 12, 2017

domenic commented Oct 12, 2017

domenic commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Oct 17, 2017

domenic commented Oct 20, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Oct 27, 2017

domenic commented Oct 31, 2017

ricea commented Nov 1, 2017

ricea commented Nov 1, 2017

ricea commented Nov 2, 2017

TejasQ Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasQ commented Nov 2, 2017

ricea commented Sep 26, 2017 •

edited by pr-preview bot

Loading

TejasQ Nov 2, 2017 •

edited

Loading