Re: [whatwg/fetch] Add abort reason to abort fetch (PR #1343)

@domenic commented on this pull request.

Sorry for the long delay. I think this is on the right path; the overall structure makes sense to me now, including the service worker PR.

I will endeavor to do future reviews within 24 hours :).

> + <li><p>Let <var>serializedError</var> be the result of calling [$StructuredSerialize$] with
+ <var>error</var>. If that threw and exception, let <var>serializedError</var> be the result of calling
+ [$StructuredSerialize$] with "{{AbortError}}" {{DOMException}}.

```suggestion
 <li><p>Let <var>serializedError</var> be [$StructuredSerialize$](<var>error</var>). If that threw an exception, let <var>serializedError</var> be [$StructuredSerialize$](a new  "{{AbortError}}" {{DOMException}}).
```

> @@ -7785,8 +7804,8 @@ method steps are:
    <li><p>If <var>locallyAborted</var> is true, terminate these substeps.
 
    <li><p>If <var>response</var>'s <a for=response>aborted flag</a> is set, then <a>abort fetch</a>
-   with <var>p</var>, <var>request</var>, and <var>responseObject</var>, and terminate these
-   substeps.
+   with <var>p</var>, <var>request</var>, <var>responseObject</var>, and <var>error</var>, and

_error_ is not declared anywhere.

>  
 <ol>
- <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+ <li><p>Let <var>serializedError</var> be the result of calling [$StructuredSerialize$]
+ with <var>error</var>. If that threw an exception, let <var>serializedError</var> be the result of
+ calling [$StructuredSerialize$] with "{{AbortError}}" {{DOMException}}.
+
+ <li><p>Set <var>controller</var>'s <a for="fetch controller">serialized abort reason</a> to

_controller_ is not declared.

Overall, I don't *think* these steps are necessary:

- If this is called from fetch() 4.1, then we are doing an early-abort; nothing will ever reach the server worker, and no controller will be involved.

- If this is called from fetch 11.2, then step 11.3 will take care of the serialization and controller-storage.

- If this is called from fetch 12.2, then... well, probably this case we do need to modify the controller, but maybe we should do it like step 11.2 + 11.3 and keep them separate? I.e. if response's aborted flag is set, then: 1. create a new "AbortError" DOMException; 2. "abort fetch"; 3. "abort controller"?

> @@ -7800,11 +7819,16 @@ method steps are:
  <li><p>Return <var>p</var>.
 </ol>
 
-<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>, and
-<var>responseObject</var>, run these steps:
+<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>,

It would help a lot to rename this something like "abort a fetch() call". I keep getting it confused with "abort" as used on fetch controllers, which is more general; this is about messing with the JavaScript objects specifically vended by fetch().

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1343#pullrequestreview-1096092521
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1343/review/1096092521@github.com>