Re: [whatwg/fetch] Http cache (#496)

annevk commented on this pull request.

This looks great. Mostly minor nits. Do we have tests for this already somewhere?

> @@ -3342,86 +3345,149 @@ steps:
 
  <li><p>Let <var>response</var> be null.
 
+ <li><p>Let the <var>revalidating</var> and <var>partial</var> flags be unset.

I think we should either make these booleans or pull flag into their name.

>  
-    <p class=note>As mandated by HTTP, this still takes the `<code>Vary</code>`
-    <a>header</a> into account.
+    <p class=note>The cache key used to locate a stored response must include the value of the

We cannot use must in notes. I think the right solution here is probably to not make this a note, apart from maybe the bit that is explaining the requirement.

>  
  <li>
+  <!-- If response is still null, we require a forward request. -->

Minor nit: "forwarded request" seems to be the term of art?

>  
- <li>
-  <p>If <var>response</var>'s <a for=response>status</a> is <code>304</code> and
-  <var>httpRequest</var>'s <a for=request>cache mode</a> is either
-  "<code>default</code>" or "<code>no-cache</code>", run these substeps:
+   <li><p>If <var>httpRequest</var>'s <var>method</var> is unsafe and <var>forwardResponse</var>'s
+   <a for=response>status</a> begins with <code>2</code> or <code>3</code>, invalidate appropriate
+   stored responses in the HTTP cache, as per the 
+   "<a href=https://tools.ietf.org/html/rfc7234#section-4.4>Invalidation</a>" chapter of 
+   <cite>HTTP Caching</cite> [[!HTTP-CACHING]], and set <var>storedResponse</var> to null.

This step looks a little scary. 1 "unsafe" can change over time. Are we expecting implementations to change their result over time? If so we should probably reference "unsafe". I guess we also want tests for this.

Also, I think I'm mostly treating status as an integer, so a range check would make more sense, e.g., in the range 200 to 399, inclusive.

>  
   <ol>
    <li>
-    <p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is
-    "<code>force-cache</code>" or "<code>only-if-cached</code>", then set
-    <var>response</var> to the <a for=/>response</a> in the HTTP cache for
-    <var>httpRequest</var>.
+    <p>Let <var>storedResponse</var> be the result of selecting a response from the HTTP cache,

I think it would be good to declare storedResponse upfront just like partial/revalidating since you end up using it outside these substeps.

>  
   <ol>
    <li>
-    <p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is
-    "<code>force-cache</code>" or "<code>only-if-cached</code>", then set
-    <var>response</var> to the <a for=/>response</a> in the HTTP cache for
-    <var>httpRequest</var>.
+    <p>Let <var>storedResponse</var> be the result of selecting a response from the HTTP cache,

And then here you just say "Set storedResponse" to the result of selecting a ..., if any." (meaning it keeps its initial value otherwise).

>  
-   <li><p>Update <var>cachedResponse</var>'s <a for=response>header list</a>
-   using <var>response</var>'s <a for=response>header list</a>, as per the
-   "<a href=https://tools.ietf.org/html/rfc7234#section-4.3.4>Freshening Stored Responses upon Validation</a>"
-   chapter of <cite>HTTP Caching</cite>. [[!HTTP-CACHING]]
+     <li><p>Set <var>response</var> to <var>storedResponse</var>.
+
+     <p class="note">This updates the stored response in cache as well.

It seems this note is more fitting for the paragraph above? Also, the formatting is a little off. If you have multiple `<p>` descendants, they need to be on their own line and indented by one.

>  
-    <p class="note no-backref">This changes <var>response</var> entirely, including its
-    <a for=response>status</a> which is most likely <code>200</code> now.
+    <ol>
+     <li><p>Update <var>storedResponse</var>'s <a for=response>header list</a> using
+     <var>forwardResponse</var>'s <a for=response>header list</a>, as per the 
+     "<a href=https://tools.ietf.org/html/rfc7234#section-3.3>Combining Partial Content</a>" chapter
+     of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
+
+     <li><p>Set <var>response</var> to <var>storedResponse</var>.
+
+     <p class="note">This updates the stored response in cache as well.

Same here. It seems this note should be for one step earlier.

>  
-    <p class="note no-backref">This changes <var>response</var> entirely, including its
-    <a for=response>status</a> which is most likely <code>200</code> now.
+    <ol>
+     <li><p>Update <var>storedResponse</var>'s <a for=response>header list</a> using
+     <var>forwardResponse</var>'s <a for=response>header list</a>, as per the 
+     "<a href=https://tools.ietf.org/html/rfc7234#section-3.3>Combining Partial Content</a>" chapter
+     of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
+
+     <li><p>Set <var>response</var> to <var>storedResponse</var>.
+
+     <p class="note">This updates the stored response in cache as well.
+    </ol>
+    <!-- TODO: account for partial request failure -->
+
+    <li><p>If <var>response</var> is null, run these substeps:

`<p>` needs to be on a new line due to the sibling `<ol>`.

> +      <li><p>If <var>forwardResponse</var> is a <a>network error</a>, <var>storedResponse</var> is
+      not null, <var>storedResponse</var> is complete, and <var>storedResponse</var> is allowed to 
+      be served stale, as per the
+      "<a href=https://tools.ietf.org/html/rfc7234#section-4.2.4>Serving Stale Responses</a>"
+      chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]]>", set <var>response</var> to
+      <var>storedResponse</var> and abort these substeps.
+
+      <li><p>Set <var>response</var> to <var>forwardResponse</var>.
+
+      <li><p>Store <var>httpRequest</var> and <var>forwardResponse</var> in the HTTP cache, as per 
+      the "<a href=https://tools.ietf.org/html/rfc7234#section-3>Storing Responses in Caches</a>"
+      chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
+
+      <p class=note>The cache key used to store the response must include the value of the
+      <var>credentials</var> flag, to ensure that a stored response to a request with credentials is
+      not used to satisfy a request without credentials.

Same comment as earlier, no requirements in notes. Also a formatting glitch here due to multiple `<p>` inside the `<li>`.

-- 
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/496#pullrequestreview-24543310