Re: [whatwg/fetch] (WIP) allow Request to outlive environment settings object (#388)

annevk commented on this pull request.

Many nits, but the main problem I see here is that you haven't defined what size of an object means. As I understand it from a discussion with @esprehn and others, calculating the size of `FormData` requires a roundtrip through networking land. Whereas this would require that to be a synchronous lookup.

> @@ -4224,7 +4224,7 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
 
 <p>To <dfn title=concept-BodyInit-extract>extract</dfn> a <span title=concept-body>body</span> and a
 `<code title>Content-Type</code>` <span title=concept-header-value>value</span> from
-<var>object</var>, run these steps:
+<var>object</var> with optional <var>keepalive-body-size</var>, run these steps:

`keepaliveBodySize` (`Content-Type` is an exception that should maybe be modified at some point).

> @@ -4487,6 +4493,7 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   readonly attribute <span>RequestCache</span> <span title=dom-Request-cache>cache</span>;
   readonly attribute <span>RequestRedirect</span> <span title=dom-Request-redirect>redirect</span>;
   readonly attribute DOMString <span title=dom-Request-integrity>integrity</span>;
+  readonly attribute boolean <span title=dom-Request-keepAlive>keepAlive</span>;

keepAlive or keepalive? We should stay consistent. https://en.wikipedia.org/wiki/Keepalive suggests the time has come where it is one word, but HTTP sets precedent in the other direction unfortunately.

>   <span title=concept-request-integrity-metadata>integrity metadata</span> is
  <var>request</var>'s
- <span title=concept-request-integrity-metadata>integrity metadata</span>.
+ <span title=concept-request-integrity-metadata>integrity metadata</span>, and
+ <span title=concept-request-keep-alive-flag>keep-alive flag</span> is

This flag hasn't been defined it seems?

> @@ -4734,6 +4746,10 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
  <var>request</var>'s
  <span title=concept-request-integrity-metadata>integrity metadata</span> to it.
 
+ <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present, set

then set*

> @@ -4291,6 +4291,12 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   </dl>
 
  <li>
+  <p>If <var>keepalive-body-size</var> is provided and <var>object</var>'s type is
+  <code title=concept-ReadableStream>ReadableStream</code>, or <var>object</var>'s size is greater
+  than <var>keepalive-body-size</var>, throw a <code title>TypeError</code> and abort the

then throw* and no need to say the remaining steps are aborted. That follows from throwing (new convention we're trying to use).

> @@ -4802,7 +4818,17 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   <ol>
    <li><p>Let <var>Content-Type</var> be null.
 
-   <li><p>Set <var>inputBody</var> and <var>Content-Type</var> to the result of
+   <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present and is set to
+   <code>true</code>, then set <var>inputBody</var> and <var>Content-Type</var> to the
+   result of <span title=concept-BodyInit-extract>extracting</span> <var>init</var>'s
+   <code title>body</code> member with <var>keepalive-body-size</var> set by user agent policy.

I suggest we introduce keepalive-body-size in an earlier step so it's more clear it's user-agent defined.

> @@ -4802,7 +4818,17 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   <ol>
    <li><p>Let <var>Content-Type</var> be null.
 
-   <li><p>Set <var>inputBody</var> and <var>Content-Type</var> to the result of
+   <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present and is set to

You need a newline after the `<li>` since it contains two `<p>`s.

> @@ -4802,7 +4818,17 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   <ol>
    <li><p>Let <var>Content-Type</var> be null.
 
-   <li><p>Set <var>inputBody</var> and <var>Content-Type</var> to the result of
+   <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present and is set to
+   <code>true</code>, then set <var>inputBody</var> and <var>Content-Type</var> to the
+   result of <span title=concept-BodyInit-extract>extracting</span> <var>init</var>'s
+   <code title>body</code> member with <var>keepalive-body-size</var> set by user agent policy.
+   Rethrow any exceptions.
+   <p class="note no-backref">This step ensures that requests that are allowed to outlive the

Newline before the `<p>` and probably also should be indented one space more.

> @@ -4291,6 +4291,12 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   </dl>
 
  <li>
+  <p>If <var>keepalive-body-size</var> is provided and <var>object</var>'s type is
+  <code title=concept-ReadableStream>ReadableStream</code>, or <var>object</var>'s size is greater

a `ReadableStream` object*

> @@ -4291,6 +4291,12 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
   </dl>
 
  <li>
+  <p>If <var>keepalive-body-size</var> is provided and <var>object</var>'s type is
+  <code title=concept-ReadableStream>ReadableStream</code>, or <var>object</var>'s size is greater

Also, how do we determine _object_'s size? As I understand it that can be quite hard for `FormData`.

-- 
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/388#pullrequestreview-2291788