Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize behavior of maxAgeSeconds #3317

Open
atjn opened this issue Apr 25, 2024 · 0 comments
Open

Standardize behavior of maxAgeSeconds #3317

atjn opened this issue Apr 25, 2024 · 0 comments

Comments

@atjn
Copy link

atjn commented Apr 25, 2024

Hello new team :)

I found a bug in #2863 (comment) and decided now that it should probably have a separate issue.

Library Affected:
workbox-expiration.

Issue or Feature Request Description:

The documentation for what maxAgeSeconds does, is pretty vague:

The maximum age of an entry before it's treated as stale and removed.

...so I did some manual testing and came to the conclusion that it must be calculating the "age" as the time since the resource was downloaded form the server. And because of that assumption, I created an entire feature request for maxUnusedSeconds which calculates the age as the time since the resource was last used on the page.

But now it turns out, the existing implementation is actually doing a bit of both. The lightweight Date check you're doing in isResponseDateFresh is correctly determining the time since the resource was last downloaded from the server, but for the IndexedDB check in expireEntries to also work correctly, the timestamp must only be updated when a new version of the resource is downloaded from the server, and that is not currently the case, since you call updateTimestamp as part of cachedResponseWillBeUsed:

// Update the metadata for the request URL to the current timestamp,
// but don't `await` it as we don't want to block the response.
const updateTimestampDone = cacheExpiration.updateTimestamp(request.url);

In practice, this means that the behavior of the ExpirationPlugin can drastically change based on subtle differences. As an example, if you save your images with the CacheFirst strategy and set maxAgeSeconds to a month, then as long as all your images are served with a correct Date header, they will all be removed and get a fresh update from the server at least once a month. But if you one day accidentally disable the Date header on your server, then the serviceworker will fall back to IndexedDB timestamps, which are updated every time the images are used, so if a user watches those images at least once a month for several years, then the images will just stay in cache and never be redownloaded from the server for all those years.

I believe the expected behavior is to always calculate the "age" as the time since the resource was downloaded form the server, and therefore you should make the IndexedDB calls adhere to that standard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant