Re: [whatwg/fetch] Attach timing info and URL to network errors, and report for fetch API (#1311)

@annevk commented on this pull request.

Using streams in this way seems really nice and reduces the overall complexity a lot. Great work! I do think there are some logic errors here and there, but they seem fixable.

> @@ -4200,6 +4200,60 @@ steps:
 <a for=/>response</a> <var>response</var>, run these steps:
 
 <ol>
+ <li>
+  <p>If <var>response</var> is a <a>network error</a>, then:
+
+  <ol>
+   <li>
+    <p>Set <var>response</var>'s <a for=response>URL list</a> to « <var>fetchParams</var>'s
+    <a for="fetch params">request</a>'s <a for=request>URL list</a> ≫.

There's two problems here. The list closing code point is incorrect and the contents of the list is another list? I thought we only wanted to reveal the initial URL?

> +   <a for="fetch params">process response done</a> given <var>response</var> with
+   <var>fetchParams</var>'s <a for="fetch params">task destination</a>.
+  </ol>
+ </li>
+
+ <li>
+  <p>If <var>hasReadableStream</var> is true, then:
+
+  <ol>
+   <li><p>Let <var>transformStream</var> be a new a {{TransformStream}}.
+
+   <li><p>Let <var>identityTransformAlgorithm</var> be an algorithm which, given <var>chunk</var>,
+   <a for=TransformStream lt=enqueue>enqueues</a> <var>chunk</var> in <var>transformStream</var>.
+
+   <li><p><a for=TransformStream lt="setting up">Set up</a> <var>transformStream</var> with
+   <var>identityTransformAlgorithm</var> and <var>processResponseDone</var>.

Nit: let's use the named arguments here for clarity. It wasn't immediately clear to me the second algorithm would only be run once. (https://infra.spec.whatwg.org/#algorithm-params explains this.)

> @@ -4210,12 +4264,23 @@ steps:
   then:
 
   <ol>
+   <li>
+    <p>Let <var>processEndOfBody</var> given <var>nullBytesOrFailure</var> be the following
+    steps:
+
+    <ol>
+     <li><p>Run <var>processResponseDone</var>.

Why is this needed? Wouldn't this run as part of reading the body?

> +    <p>Set <var>response</var>'s <a for=response>URL list</a> to « <var>fetchParams</var>'s
+    <a for="fetch params">request</a>'s <a for=request>URL list</a> ≫.
+
+    <p class=note>This is needed as after <a for=list>cloning</a> <var>fetchParams</var>'s
+    <a for="fetch params">request</a>'s <a for=request>URL list</a> earlier, <var>response</var>
+    might have been set to a <a>network error</a>.</p>
+   </li>
+
+   <li><p>Set <var>response</var>'s <a for=response>timing info</a> to the result of
+   <a>creating an opaque timing info</a> for <var>fetchParams</var>'s
+   <a for="fetch params">timing info</a>.</p></li>
+  </ol>
+ </li>
+
+ <li><p>Let <var>hasReadableStream</var> be true if <var>response</var>'s <a for=response>body</a>
+ is not null and is <a for=ReadableStream>readable</a>; otherwise false.

I think it should always be a stream if body is non-null.

>  
- <li><p>If <var>fetchParams</var>'s <a for="fetch params">process response done</a> is not null,
- then <a>queue a fetch task</a> to run <var>fetchParams</var>'s
- <a for="fetch params">process response done</a> given <var>response</var>,
- with <var>fetchParams</var>'s <a for="fetch params">task destination</a>.
+  <li><p>If <var>hasReadableStream</var> is false, then run <var>processResponseDone</var>.</p></li>

We already run this above when response's body is null (through _processBody_), so it's not clear to me this is needed.

> + <li>
+  <p>If <var>hasReadableStream</var> is true, then:
+
+  <ol>
+   <li><p>Let <var>transformStream</var> be a new a {{TransformStream}}.
+
+   <li><p>Let <var>identityTransformAlgorithm</var> be an algorithm which, given <var>chunk</var>,
+   <a for=TransformStream lt=enqueue>enqueues</a> <var>chunk</var> in <var>transformStream</var>.
+
+   <li><p><a for=TransformStream lt="setting up">Set up</a> <var>transformStream</var> with
+   <var>identityTransformAlgorithm</var> and <var>processResponseDone</var>.
+
+   <li><p>Set <var>response</var>'s <a for=response>body</a> to the result of
+   <a for=ReadableStream lt="piping through">piping</a> <var>response</var>'s
+   <a for=response>body</a> through <var>transformStream</var>.
+  </ol>

Let's add a note after this `</ol>` explaining that we are doing this to get a notification after at end-of-stream.

-- 
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/1311#pullrequestreview-822462495