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

PWA Discussion: Caching configuration #204

Closed
ryelle opened this issue Aug 7, 2019 · 5 comments
Closed

PWA Discussion: Caching configuration #204

ryelle opened this issue Aug 7, 2019 · 5 comments
Labels
[Component] PWA Service workers, manifest

Comments

@ryelle
Copy link
Contributor

ryelle commented Aug 7, 2019

from comments

There are questions about how these caching methods work for site assets, if they're good defaults and how they work with expirations.

  • Assets are limited to 60 entries and are set to invalidate after a day. Assets will be pulled from the cache by default, if they exist there is no network request. We might want to bump the number of entries since there can be a lot of CSS/JS & images (esp with sponsors) on a page.
  • Custom CSS is also cached with 60 entries up to a day, but since the query value is part of the cache route rule, as soon as the query updates, the service worker's caching rules change. This essentially deletes the old cache rule, and adds a new one for the current custom CSS. We could probably tweak the rules here to cache this indefinitely, since it will invalidate itself.
  • API requests are also cached, for 15min. Again, the response will be pulled from the cache if it exists, and skip the network request until it's invalid. Why did it seem necessary to cache the API?
  • Navigation pages are limited to 50 entries, and don't time out, but will only request from cache if offline. PWA Discussion: Caching strategy for navigation (pages a user navigates too) #202 has more about this.
  • There are some things we're not caching– Remote CSS PWA: Add caching for remote CSS #200, uploaded files PWA: Add caching for uploaded files #203

Cachebuster strings do not invalidate cached entries. If the resource changes, the ETag would change, but this doesn't seem to help. In the case of the assets, this doesn't trigger a change since the network is never hit when a cached value is found. (this might need more research)

As for side effects, the comments call out Gutenberg & Tagregator. It shouldn't affect gutenberg since the admin service worker is separate and removes any caching from the API.

Caching the API request does "break" tagregator– it continues to ping the API every 30s, but gets the same cached value back for 15min. IMO, tagregator doesn't really make sense in an offline context, so we should skip caching it - if we're worried about bandwidth, we can bump the interval to something in 1-5min, but otherwise this caching is causing JS overhead (as the request is fetched and processed at the tggr interval) without having real "real-time" updates.

So to summarize, the questions here are about whether these caching settings sound appropriate.

@iandunn
Copy link
Member

iandunn commented Sep 25, 2019

API requests are also cached, for 15min. Again, the response will be pulled from the cache if it exists, and skip the network request until it's invalid. Why did it seem necessary to cache the API?

Looks like it was added in wceu/wordcamp-pwa-page@b15ad18

Maybe it was to avoid traffic spikes that the server can't handle? It does seem a bit aggressive. @avillegasn, @westonruter - do you remember?

If that was the reason, then server-side caching would probably be more appropriate, but we should get that in place before disabling the JS caching, especially if we enable this for WCUS.

Caching the API request does "break" tagregator– it continues to ping the API every 30s, but gets the same cached value back for 15min. IMO, tagregator doesn't really make sense in an offline context, so we should skip caching it - if we're worried about bandwidth, we can bump the interval to something in 1-5min, but otherwise this caching is causing JS overhead (as the request is fetched and processed at the tggr interval) without having real "real-time" updates.

I agree. Nginx is caching the Tagregator endpoint for 30 seconds, so we shouldn't have to worry about overloading the server there.

@avillegasn
Copy link

avillegasn commented Sep 26, 2019

AFAIK API requests were cached to avoid traffic spikes. In case you don't want to cache all endpoints, you could refine the first parameter here to be more specific instead of /wp-json/.*.

@ryelle
Copy link
Contributor Author

ryelle commented Sep 30, 2019

AFAIK API requests were cached to avoid traffic spikes.

Is that because the API was repeated pinged by the Day Of Event template? That had 3 API requests per minute. In that case, we'll address that when we address #207 (probably as part of #243/#241, both still drafts).

If nginx is already caching tagregator, it sounds like once the other issues are taken care of we can remove the API caching.

@westonruter
Copy link
Member

Maybe it was to avoid traffic spikes that the server can't handle? It does seem a bit aggressive.

The purpose was to prevent eating up the user's bandwidth with REST API calls, as I remember. So this was a short-term workaround for adding proper conditional requests in the REST API endpoint. Ideally the REST API response would include a Last-Modified header whose value the client would then send as part of a If-Modified-Since request header, allowing the server to respond with 304 Not Modified, which would result in an empty response body and minimal network bandwidth. See wceu/wordcamp-pwa-page#6 (comment)

@ryelle
Copy link
Contributor Author

ryelle commented Oct 3, 2019

Some housekeeping, since there's a lot going on in this description…

Once #244 is merged, this can be closed, since the other topics are in other issues now.

@ryelle ryelle closed this as completed Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] PWA Service workers, manifest
Projects
None yet
Development

No branches or pull requests

4 participants