Re: [whatwg/fetch] Return a "controller" instance from fetch (#1329)

@jakearchibald requested changes on this pull request.

Just a few nits really. 

> @@ -255,6 +258,29 @@ given a <a for=/>fetch timing info</a> <var>timingInfo</var>, return a new
  <var>storedTimingInfo</var>'s <a for="fetch timing info">decoded body size</a>.
 </ol>
 
+<p>To <dfn for="fetch params">abort</dfn> a <a for=/>fetch params</a> <var>fetchParams</var>,
+set <var>fetchParams</var>'s <a for="fetch params">fetch state</a> to "<code>aborted</code>".
+
+<p>To <dfn for="fetch params">terminate</dfn> a <a for=/>fetch params</a> <var>fetchParams</var>,
+set <var>fetchParams</var>'s <a for="fetch params">fetch state</a> to "<code>terminated</code>".
+
+<p>A <a for=/>fetch params</a> <var>fetchParams</var> is <dfn for="fetch params">canceled</dfn> if
+its <a for="fetch params">fetch state</a> is "<code>aborted</code>" or "<code>terminated</code>".
+
+<p>A <dfn export>fetch controller</dfn> is an object used to enable clients of an ongoing fetch

@annevk I've always tried to avoid calling spec constructs 'objects' to avoid confusion with platform objects, or is this not actually a problem?

> +<p>A <a for=/>fetch params</a> <var>fetchParams</var> is <dfn for="fetch params">canceled</dfn> if
+its <a for="fetch params">fetch state</a> is "<code>aborted</code>" or "<code>terminated</code>".
+
+<p>A <dfn export>fetch controller</dfn> is an object used to enable clients of an ongoing fetch
+to perform certain actions in the context of that fetch instance.
+
+<p>A <a for=/>fetch controller</a> has an associated <a for=/>fetch params</a>
+<dfn for="fetch controller">ongoing fetch params</dfn>.</p>
+
+<p>To <dfn export id="concept-fetch-terminate" for="fetch controller">terminate</dfn> a
+<a for=/>fetch controller</a> <var>controller</var>, <a for="fetch params">terminate</a>
+<var>controller</var>'s <a for="fetch controller">ongoing fetch params</a>.
+
+<p>To <dfn for="fetch controller">abort</dfn> a <a for=/>fetch controller</a>
+<var>controller</var>, <a for="fetch params">abort</a> <var>controller</var>'s
+<a for="fetch controller">ongoing fetch params</a>.

Nit: Is 'ongoing' the right name here, considering it might be completed, aborted, or terminated?

> @@ -255,6 +258,29 @@ given a <a for=/>fetch timing info</a> <var>timingInfo</var>, return a new
  <var>storedTimingInfo</var>'s <a for="fetch timing info">decoded body size</a>.
 </ol>
 
+<p>To <dfn for="fetch params">abort</dfn> a <a for=/>fetch params</a> <var>fetchParams</var>,
+set <var>fetchParams</var>'s <a for="fetch params">fetch state</a> to "<code>aborted</code>".
+
+<p>To <dfn for="fetch params">terminate</dfn> a <a for=/>fetch params</a> <var>fetchParams</var>,
+set <var>fetchParams</var>'s <a for="fetch params">fetch state</a> to "<code>terminated</code>".
+
+<p>A <a for=/>fetch params</a> <var>fetchParams</var> is <dfn for="fetch params">canceled</dfn> if
+its <a for="fetch params">fetch state</a> is "<code>aborted</code>" or "<code>terminated</code>".
+
+<p>A <dfn export>fetch controller</dfn> is an object used to enable clients of an ongoing fetch
+to perform certain actions in the context of that fetch instance.

A fetch controller only has fetch params, and only performs actions that can be performed on fetch params, so I'm struggling to see the point in it.

Why is it a separate thing?

> @@ -2065,6 +2091,11 @@ known as an <dfn export id=concept-aborted-network-error>aborted network error</
 <a for=response>header list</a> is always empty, and
 <a for=response>body</a> is always null.
 
+<p>To create the <dfn>appropriate network error</dfn> given <a for=/>fetch params</a>
+<var>fetchParams</var>, return an <a>aborted network error</a> if <var>fetchParams</var>'s
+<a for="fetch params">fetch state</a> is "<code>aborted</code>"; otherwise return a
+<a>network error</a>.

Is it worth adding an assert that the fetch state isn't "ongoing"?

> @@ -2247,14 +2278,14 @@ functionality.
 
 <p>A <a for="fetch group">fetch record</a> has an associated
 <dfn export for="fetch record" id=concept-fetch-record-fetch>fetch</dfn> (a
-<a for=/>fetch</a> algorithm or null).
+<a for=/>fetch controller</a> or null).

I can't see any cases where it's null.

>  
 <hr>
 
 <p>When a <a for=fetch>fetch group</a> is
 <dfn export for="fetch group" id=concept-fetch-group-terminate>terminated</dfn>, for each associated
 <a for="fetch group">fetch record</a> whose <a for="fetch record">request</a>'s <a>done flag</a> is
-unset or <a for=request>keepalive</a> is false, <a lt=terminated for=fetch>terminate</a> the
+unset or <a for=request>keepalive</a> is false, <a for="fetch controller">terminate</a> the
 <a for="fetch group">fetch record</a>'s <a for="fetch record">fetch</a>.

If the fetch can actually be null, then this bit is fragile.

> @@ -3816,13 +3846,15 @@ the request.
 
   <ol>
    <li><p>Let <var>record</var> be a new <a for="fetch group">fetch record</a> consisting of
-   <var>request</var> and this instance of the <a for=/>fetch</a> algorithm.
+   <var>request</var> and <var>controller</var>.

Personally, I'd rather this referenced the properties of fetch record explicitly, so it turns up in the spec when you look for references to `fetch record/fetch`. WDYT @annevk?

> @@ -205,6 +205,9 @@ lt="authentication entry">authentication entries</a> (for HTTP authentication).
  <dt><dfn for="fetch params">cross-origin isolated capability</dfn> (default false)
  <dd>A boolean.
 
+ <dt><dfn for="fetch params">fetch state</dfn>
+ <dd>"<code>ongoing</code>", "<code>terminated</code>", or "<code>aborted</code>".

This needs to be given a default value, or the value needs to be set whenever fetch params are created.

>    </ol>
 
  <li><p>Let <var>pullAlgorithm</var> be an action that <a lt=resumed for=fetch>resumes</a> the
  ongoing fetch if it is <a lt=suspend for=fetch>suspended</a>.
 
- <li><p>Let <var>cancelAlgorithm</var> be an action that <a lt=terminated for=fetch>terminates</a>
- the ongoing fetch with the aborted flag set.
+ <li><p>Let <var>cancelAlgorithm</var> be to <a for="fetch params">abort</a> <var>fetchParams</var>.

@annevk is this the right wording for creating an algorithm? It sounds a but clumsy.

-- 
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/fetch/pull/1329#pullrequestreview-790426284