Add more cross-links #872

surma · 2018-01-26T22:31:39Z

This PR does not contain functional changes.

I found the spec to be a bit frustrating to browse when I was trying to figure out which methods are on a stream, a stream controller or the underlying sink/source. I am linkifying a couple of things to make the spec more navigational.

Please mind that this is my first work with bikeshed so I might not be doing it right, don’t hesitate to make me correct things.

PTAL :)

Preview | Diff

surma · 2018-01-26T22:32:23Z

index.bs

@@ -444,11 +444,11 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
  govern how the constructed stream instance behaves:

  <ul>
-    <li><p><code>start(controller)</code> is called immediately, and is typically used to adapt a <a>push
+    <li><p><code>start({{ReadableStreamDefaultController|controller}})</code> is called immediately, and is typically used to adapt a <a>push


This one is a bit questionable because it could also be a BYOB, but I think having it clickable is worth the simplification.

domenic

Some good stuff here, thanks!

In addition to specific issues noted, let's be sure the final patch rewraps to the 120-character column limit.

domenic · 2018-01-27T04:48:56Z

index.bs

@@ -95,7 +95,7 @@ A <dfn export>readable stream</dfn> represents a source of data, from which you
 <em>out</em> of a readable stream. Concretely, a readable stream is an instance of the {{ReadableStream}} class.

 Although a readable stream can be created with arbitrary behavior, most readable streams wrap a lower-level I/O source,
-called the <dfn>underlying source</dfn>. There are two types of underlying source: push sources and pull sources.
+called the <dfn>underlying source</dfn> ([[#rs-constructor|interface description]]). There are two types of underlying source: push sources and pull sources.


This isn't good, because we're currently talking about the general concept of underlying sources, not the JS objects that sometimes (for web-developer-created streams) embody them. Such a link belongs later, where we say "For web developer–created streams, the implementation details of a source are provided by an object with certain methods and properties that is passed to the ReadableStream() constructor." But, that already links to the constructor, so I'm not sure if there's anything to do here.

I see your point. The problem that I’m trying to solve here is the following flow:

Go to stream spec

Click “ReadableStream” in ToC

Get confused, maybe click ReadableStream again?

Okay, I’m at the class, I see underlyingSource, but I can’t click it. The paragraph has a clickable “underlying Source”, tho. So let’s click that.

I was already here...

Maybe we can turn underlyingSource in the class definition into a link to the note? Would that be acceptable?

domenic · 2018-01-27T04:49:03Z

index.bs

@@ -137,7 +137,7 @@ A <dfn export>writable stream</dfn> represents a destination for data, into whic
 goes <em>in</em> to a writable stream. Concretely, a writable stream is an instance of the {{WritableStream}} class.

 Analogously to readable streams, most writable streams wrap a lower-level I/O sink, called the
-<dfn>underlying sink</dfn>. Writable streams work to abstract away some of the complexity of the underlying sink, by
+<dfn>underlying sink</dfn> ([[#ws-constructor|interface description]]). Writable streams work to abstract away some of the complexity of the underlying sink, by


Same problem as above.

domenic · 2018-01-27T04:49:49Z

index.bs

@@ -385,15 +385,15 @@ like

 <pre><code class="lang-javascript">
  class ReadableStream {
-    constructor(underlyingSource = {}, { size, highWaterMark } = {})
+    <l>[[#rs-constructor|constructor]]</l>(underlyingSource = {}, { size, highWaterMark } = {})


Personally I find this more confusing than just doing <a href="#rs-constructor">constructor</a>. Does that work, or is only <l> allowed inside <pre> blocks?

Just tested it. Works as well.

domenic · 2018-01-27T04:50:47Z

index.bs

@@ -444,11 +444,11 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
  govern how the constructed stream instance behaves:

  <ul>
-    <li><p><code>start(controller)</code> is called immediately, and is typically used to adapt a <a>push
+    <li><p><code>start({{ReadableStreamDefaultController|controller}})</code> is called immediately, and is typically used to adapt a <a>push


I'm not really OK with linking variable names to their types in general, here or elsewhere. (Especially, as you say, when their types are not accurate.) If you think it's unclear what the type of the controller is, we should add a more explicit sentence explaining.

This occurs several times below; I'll avoid noting it each time.

That’s fair. I’ll take a stab at rephrasing the text so I can add some links to that instead.

domenic · 2018-01-27T04:51:34Z

index.bs

@@ -385,15 +385,15 @@ like

 <pre><code class="lang-javascript">


The introduction of links into these blocks is a great improvement; thank you!

surma · 2018-01-27T13:21:14Z

Fixed line length and moved some of the links away from the code blocks into the descriptive text. PTAL :)

surma · 2018-01-27T13:22:25Z

index.bs


    <li>
      <p><code>transform(chunk, controller)</code> is called when a new chunk originally written to the writable side is
      ready to be transformed. It can return a promise to signal success or failure of the transformation. The results
      of the transformation can be enqueued to the readable side using the
-      {{ReadableStreamDefaultController/enqueue(chunk)|controller.enqueue()}} method. This permits a single chunk
-      written to the writable side to result in zero or multiple chunks on the readable side.
+      {{TransformStreamDefaultController|controller}}.{{TransformStreamDefaultController/enqueue(chunk)|enqueue()}}


What do you think about splitting the links to contain both the class definition as well as the method definition?

Additionally, was it intentional that this linked to ReadableStreamDefaultController.enqueue() instead of TransformStreamDefaultController.enqueue()

I'm 99% sure that linking to ReadableStreamDefaultController.enqueue() was a mistake here. Thanks for catching it.

Splitting the links looks okay to me, but I will defer to @domenic who knows much more about correct markup fro WhatWG standards.

surma · 2018-01-27T15:32:59Z

index.bs

@@ -385,15 +385,16 @@ like

 <pre><code class="lang-javascript">
  class ReadableStream {
-    constructor(underlyingSource = {}, { size, highWaterMark } = {})
+    <a href="#rs-constructor">constructor</a>(<a href="#rs-constructor">underlyingSource</a> = {}, { size, highWaterMark


I realize linking underlyingSource to ReadableStream() could be seen as questionable. This is just as suggestion. My thinking was that if someone’s looking at the class definition and the constructor, they are most likely a web developer and looking for an interface description. There’s multiple links to underlyingSource in the note, so there’s still a path to the definition.

domenic

Sorry for the delay. A few minor nits, and suggestion for a larger fix that I (or you) can work on as a follow-up.

domenic · 2018-01-31T17:17:41Z

index.bs

-web developer–created streams, the implementation details of a source are provided by an object with certain methods and
-properties that is passed to the {{ReadableStream()}} constructor.
+Readable streams are designed to wrap both types of sources behind a single, unified interface. For web
+developer–created streams, the <a href="#rs-constructor">implementation details of a source</a> are provided by an


Still not sure this helps that much compared to just clicking on the already-existing "ReadableStream() constructor" link, but I'm OK keeping it if you think so.

It mostly because of the point below, that as a reader it’s not obvious that the underlying source is defined at the constructor. I’d prefer leaving this link for now until we give underlyingSource its own section.

domenic · 2018-01-31T17:19:00Z

index.bs

@@ -385,15 +385,15 @@ like

 <pre><code class="lang-javascript">
  class ReadableStream {
-    constructor(underlyingSource = {}, { size, highWaterMark } = {})
+    <a href="#rs-constructor">constructor</a>(<a href="#rs-constructor">underlyingSource</a> = {}, { size, highWaterMark } = {})


Again the second link to the exact same place feels pretty redundant.

Maybe what this is pointing toward is the need for a dedicated section describing the underlying source objects, instead of putting it into the constructor's intro note? I guess same for the strategies too? I'd be happy to work on that as a follow-up. In the meantime I'm indifferent as to whether we include these redundant links or not.

The more I work on this, the more I think that’s the actual crux: Something as integral to developing streams shouldn’t just be defined in a note. So I’m up for formalizing underlyingSource and underlyingSink, but defo need some guidance what the right formalism is here.

I’ll remove the links.

domenic · 2018-01-31T17:19:44Z

index.bs

@@ -2778,12 +2778,13 @@ like

 <pre><code class="lang-javascript">
  class WritableStream {
-    constructor(underlyingSink = {}, { size, highWaterMark = 1 } = {})
+    <a href="#ws-constructor">constructor</a>(<a href="#ws-constructor">underlyingSink</a> = {}, { size, highWaterMark =
+    1 } = {})


I forgot that line wrapping is not a good idea inside <pre> blocks; sorry, we should undo this.

domenic · 2018-01-31T17:21:14Z

index.bs

-      that will be read from the <a>readable side</a> but don't depend on any writes to the <a>writable side</a>. If
-      this process is asynchronous, it can return a promise to signal success or failure.
+      using
+      {{TransformStreamDefaultController|controller}}.{{TransformStreamDefaultController/enqueue(chunk)|enqueue()}}.


I'd rather not have separate links for the controller since that's just a variable name, and it is described below. So just changing the linking text from enqueue() to controller.enqueue() is probably best here.

domenic · 2018-01-31T17:31:22Z

index.bs


    <li>
      <p><code>transform(chunk, controller)</code> is called when a new chunk originally written to the writable side is
      ready to be transformed. It can return a promise to signal success or failure of the transformation. The results
      of the transformation can be enqueued to the readable side using the
-      {{ReadableStreamDefaultController/enqueue(chunk)|controller.enqueue()}} method. This permits a single chunk
-      written to the writable side to result in zero or multiple chunks on the readable side.
+      {{TransformStreamDefaultController|controller}}.{{TransformStreamDefaultController/enqueue(chunk)|enqueue()}}


Also change this one?

domenic

I forgot to mention we should probably also cross-link the other class definitions in the same way you did for the big three. But, also fine as a follow-up :)

Thanks so much!

In particular, the underlying source API, the underlying sink API, the transformer API, and queuing strategy objects. This is inspired by some of the discussion in #872. The new locations for these definitions allow us to link to them, and the more spacious format allows us to flesh out more aspects of the behavior of each option. In particular, this tries to clarify how the return value of an underlying sink's write() method is used, per conversations with Lennart Grahl.

Add more cross-links

826fe9f

surma commented Jan 26, 2018

View reviewed changes

domenic requested changes Jan 27, 2018

View reviewed changes

Move some of the links into descriptive text

c5c106a

surma commented Jan 27, 2018

View reviewed changes

Can't wrap code blocks

afe4805

domenic requested changes Jan 31, 2018

View reviewed changes

domenic reviewed Jan 31, 2018

View reviewed changes

Review by @domenic

322ad72

surma force-pushed the moar-links branch from bba6a42 to 322ad72 Compare January 31, 2018 17:32

domenic approved these changes Jan 31, 2018

View reviewed changes

domenic merged commit d7a57bb into whatwg:master Jan 31, 2018

This was referenced Mar 28, 2018

Linkify all class definitions #913

Merged

Consider better styling for API intro notes #907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more cross-links #872

Add more cross-links #872

surma commented Jan 26, 2018 •

edited by pr-preview bot

Loading

surma Jan 26, 2018

domenic left a comment

domenic Jan 27, 2018

surma Jan 27, 2018

domenic Jan 27, 2018

domenic Jan 27, 2018

surma Jan 27, 2018

domenic Jan 27, 2018

surma Jan 27, 2018

domenic Jan 27, 2018

surma commented Jan 27, 2018

surma Jan 27, 2018

surma Jan 27, 2018

ricea Jan 29, 2018

surma Jan 27, 2018

domenic left a comment

domenic Jan 31, 2018

surma Jan 31, 2018

domenic Jan 31, 2018

surma Jan 31, 2018

domenic Jan 31, 2018

domenic Jan 31, 2018

domenic Jan 31, 2018

domenic left a comment

		@@ -385,15 +385,15 @@ like

		<pre><code class="lang-javascript">

Add more cross-links #872

Add more cross-links #872

Conversation

surma commented Jan 26, 2018 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surma commented Jan 27, 2018

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

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

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

surma commented Jan 26, 2018 •

edited by pr-preview bot

Loading