search

GoogleChromeLabs / sw-toolbox

[Deprecated] A collection of service worker tools for offlining runtime requests
https://developers.google.com/web/tools/workbox/guides/migrations/migrate-from-sw
Apache License 2.0
3.62k stars 331 forks source link

Explicitly check the Date: header of a cached response when maxAge is set #206

Closed jeffposnick closed 7 years ago

jeffposnick commented 7 years ago

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

gauntface commented 7 years ago

This looks ok to me.

jeffposnick commented 7 years ago

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.

adityapunjani commented 7 years ago

@jeffposnick Sure, I will test Flipkart Lite with this branch and get back to you.

jeffposnick commented 7 years ago

Thanks, @adityapunjani! Looking forward to hearing back.

jeffposnick commented 7 years ago

I realized that we never merged this.

@adityapunjani, were you able to test out your codebase with this branch to confirm functionality?

adityapunjani commented 7 years ago

@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?

adityapunjani commented 7 years ago

@jeffposnick Thoughts?

jeffposnick commented 7 years ago

Sorry, I lost track of this.

For opaque responses, the behavior is going to be the same as it was before this patch, so I don't think there would be any regression. There's nothing better that could be done.

I'll go ahead with fixing the Travis CI build, merging, and then cutting a new release.