Re: [whatwg/fetch] Add Stale While Revalidate Handling (#853)

jakearchibald requested changes on this pull request.

Feels like this is heading in the right direction.

>  
         <p class=note>See also the
         "<a href=https://tools.ietf.org/html/rfc7234#section-4.3.4>Sending a Validation Request</a>"
         chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
 
-       <li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set
+        <li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set

This is the second "otherwise" to this if statement. Maybe the previous one is meant to be an otherwise-if, but it isn't clear without nesting.

>          <ol>
-         <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
-         <a for="header list">contains</a> `<code>ETag</code>`, then <a for="header list">append</a>
-         `<code>If-None-Match</code>` with its value to <var>httpRequest</var>'s
-         <a for=request>header list</a>.
-
-         <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
-         <a for="header list">contains</a> `<code>Last-Modified</code>`, then
-         <a for="header list">append</a> `<code>If-Modified-Since</code>` with its value to
-         <var>httpRequest</var>'s <a for=request>header list</a>.
+         <li><p>Set <var>response</var> to <var>storedResponse</var>.
+         <li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
+         <li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
+          <ul class=brief>
+          <li><p><i>stale content mode</i> set to "<code>disallow</code>" and

Does a cache mode of "reload" work better here?

It may also be worth adding a note that the response isn't use other than to populate the cache.

If the user closes the tab, should this request be canceled?

> @@ -1901,6 +1910,13 @@ is a <a>filtered response</a> whose
  <li><p>Return <var>newResponse</var>.
 </ol>
 
+<p>A <dfn export id=concept-stale-response>stale response</dfn> is a <a for=/>response</a> where
+the freshness lifetime calculation (excluding the stale-while-revalidate directive) has not
+exceeded the current age.

Isn't this the definition of a fresh response?

> @@ -1901,6 +1910,13 @@ is a <a>filtered response</a> whose
  <li><p>Return <var>newResponse</var>.
 </ol>
 
+<p>A <dfn export id=concept-stale-response>stale response</dfn> is a <a for=/>response</a> where
+the freshness lifetime calculation (excluding the stale-while-revalidate directive) has not
+exceeded the current age.
+
+<p>A <dfn export id=concept-stale-response>stale response inside revalidate window</dfn> is
+a <a>stale response</a> where the freshness lifetime calculation (including the stale-while-revalidate

I don't think this is quite right. To say an "A" is a "B", "A" must be a subset of "B", but in this case it seems the other way around.

> @@ -4163,6 +4179,21 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
    `<code>If-Range</code>`, then set <var>httpRequest</var>'s
    <a for=request>cache mode</a> to "<code>no-store</code>".
 
+   <li><p>If <var>httpRequest</var>'s <a for=request>stale content mode</a> is
+   `<code>default</code>` then:
+
+    <ol>
+     <li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>default</code>",
+     <var>httpRequest</var>'s <a for=request>method</a> is `<code>GET</code>`, and

Is this applying the "GET" restriction to other kinds of state response fetching, like "only-if-cached"?

>  
         <p class=note>See also the
         "<a href=https://tools.ietf.org/html/rfc7234#section-4.3.4>Sending a Validation Request</a>"
         chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
 
-       <li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set
+        <li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set

It might be because of the messed-up if statement, but it kinda looks like a stale response could be used even if the stale content mode is "disallow".

> @@ -1526,6 +1531,10 @@ Unless stated otherwise, it is zero.
 which is "<code>basic</code>", "<code>cors</code>", or "<code>opaque</code>".
 Unless stated otherwise, it is "<code>basic</code>".
 
+<p>A <a for=/>request</a> has an associated <dfn export for=request id=stale-content-mode>stale content mode</dfn>,
+which is "<code>allow</code>", "<code>disallow</code>" or "<code>default</code>". Unless stated
+otherwise, it is "<code>default</code>".

I'm not 100% sure, but I'm not sure this is needed. Can't the current values of "cache mode" handle this?

-- 
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/853#pullrequestreview-206407219