Fixes a bug with cacheFirst handler + maxAgeSeconds + CORS request #694
Conversation
|
Actually, as an alternative to changing |
| @@ -165,6 +166,8 @@ class CacheExpiration { | |||
| // Only return false if all the conditions are met. | |||
| return false; | |||
| } | |||
| } else { | |||
| this.expireEntries({cacheName, now}); | |||
There was a problem hiding this comment.
If there is no maxAgeSeconds, can we really regard the entry as fresh?
I'm just slightly confused at what this is solving.
There was a problem hiding this comment.
It's a little hard to line up the braces, but this else is associated with if (dateHeader). I'll clarify in a comment what's going on.
| // Only return false if all the conditions are met. | ||
| return false; | ||
| } | ||
| // will return NaN, and the comparison will always be false. We want |
There was a problem hiding this comment.
Why not just check for NaN and treat as a special case? This negation is likely to cause confusion.
There was a problem hiding this comment.
I found opting for the negation strategy here a little hard to parse. I'm similarly having trouble reasoning why we're not just special casing NaN per Matt's comment.
|
|
||
| it(`should work properly when a cache-first strategy + maxAgeSeconds is used, and responses lack a Date header`, async function() { | ||
| // See the comment later about a source of potential flakiness. | ||
| this.retries(2); |
There was a problem hiding this comment.
you should probably set the timeout of this test case to 5 or 6 with this.timeout(6 * 1000);
|
PTAL. |
| return true; | ||
| } | ||
|
|
||
| // If we have a valid headerTime, then our response is fresh iff the |
There was a problem hiding this comment.
|
Smallest nit in the world. LGTM otherwise. |
R: @addyosmani @gauntface
CC: @pbakaus
Fixes #691
This fixes a bug that would prevent a response used in a cache-first handler from properly triggering
maxAgeSeconds-based expiration iff that response lacked aDate:header. A CORS-response without theDate:header specifically whitelisted viaAccess-Control-Expose-Headerscould trigger this scenario, as @pbakaus encountered.This also changes the method signature of the
cacheWillMatchlifecycle callback to beasync(i.e. return aPromise), since we now want to wait for it to complete before returning a cached response to the client (to ensure that any expiration that's necessary gets performed). That should (arguably) be considered a breaking change, sincecacheWillMatchis part of the public interface.