Explicitly check the Date: header of a cached response when maxAge is set #206
Conversation
|
This looks ok to me. |
| // If the Date: header was invalid for some reason, parsedDate.getTime() | ||
| // will return NaN, and the comparison will always be false. That means | ||
| // that an invalid date will be treated as if the response is fresh. | ||
| if ((parsedDate.getTime() + maxAgeSeconds) < now) { |
There was a problem hiding this comment.
Since its "maxAgeSeconds", it should be (parsedDate.getTime() + maxAgeSeconds * 1000) < now)
| return cache.match(request); | ||
| return cache.match(request).then(function(response) { | ||
| var cacheOptions = options.cache || globalOptions.cache; | ||
| if (helpers.isResponseFresh(response, cacheOptions.maxAge, Date.now())) { |
There was a problem hiding this comment.
I am assuming this is a new optioncacheOptions.maxAge and not cacheOptions.maxAgeSeconds to be backward compatible with the current behaviour right?
There was a problem hiding this comment.
Ugh, no, that's just me being confused by the naming conventions used in sw-toolbox.
I'm intended to keep the same option name, so I'll update to maxAgeSeconds.
There was a problem hiding this comment.
Alright sounds good.
|
Thanks for catching my mistakes, @adityapunjani! Can you confirm that this change satisfies your use case and resolves #164? I'd like to double-check before merging. |
|
@jeffposnick Sure, I will test Flipkart Lite with this branch and get back to you. |
|
Thanks, @adityapunjani! Looking forward to hearing back. |
|
I realized that we never merged this. @adityapunjani, were you able to test out your codebase with this branch to confirm functionality? |
|
@jeffposnick Apologies for the delay. We have tested this change in production. However, I noticed it doesn't work with opaque responses as the date header cannot be accessed. We can probably merge this change along with calling out the caveat in the documentation? |
|
@jeffposnick Thoughts? |
|
Sorry, I lost track of this. For I'll go ahead with fixing the Travis CI build, merging, and then cutting a new release. |
…o maxAge-fresh-response-check
R: @adityapunjani @gauntface @addyosmani
CC: @ktmud
My current thinking is that if we release this PR, just use a minor version bump, and don't require opting-in to an additional option to turn it on. While it does change the current behavior, based on #164, it's a change that reflects the behavior that some developers had always assumed to be the case. The net effect is that entries that would have previously been valid when read from the cache are instead expired sooner, but since they're entries that would have been expired after the fact anyway, I don't see it as a breaking change.
Alongside the code review, I'd like to get some confirmation from @adityapunjani & co. that this change works for them prior to merging and cutting a new release.
Fixes #164