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

Enable HTTP caching (adds Cache-Control and Vary headers, changes some APIs from POST to GET, replaces time= with tz=) #320

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

da2x
Copy link
Contributor

@da2x da2x commented Dec 4, 2021

  • Send HTTP Vary hints to caches when varying on HTTP request headers.
  • Enable caching for static script assets for the Comment, Count, and Latest APIs. These are mostly static, except when the admin changes server setting. Clients cache for 30 min and use stale while async revalidation for 1 day.
  • Support methods other than POST and PUT in ajax.js.
  • The Count and Latest APIs responses also get short-lived cache headers and switches from POST to GET.
  • Replaces time request argument with tz and proper time zone handling (less request variability, one variant per time zone instead of one per minute of the day). Also adjusts dates as well as times to the user’s time zone.
  • Removed some ancient cache control headers and focus on the ones that work. (HTTP/1.0+ compatible.)

This only enabled caching on some assets, and not any responses that change per-user or based on cookies. It means admins may be confused by their sites not immediately changing when they change a setting that modifies these assets. I decided to focus on the end user and their experience instead. You should’t need to change settings all that often, so this is a much smaller use case anyway. A compromise could be to disable caching for the admins, but this wouldn’t be a true representation of how users experience their sites. (Admins using HashOver are therefor expected to understand caching and how to turn it off in developer tools.)

I thought long and hard about where to put the caching headers. I decided they belonged where they’re used. I figured confused developers would look at the code to figure out why a changed setting didn’t apply as expected. Greeting them with the cache header should explain it, so it’s better for code readability. I can make it a function inside Setup instead if you hate this.

Bonus sneaking in along with the others:

  • Use the modern Sec-CH-UA-Mobile client hint instead of User-Agent to determine device mobileness (when available; e.g. Edge and Chrome over HTTPS). This is part of the client hints standard and is meant to replace User-Agents in the future. Among other things, it’s better for caching (varying on one boolean vs the entire User-Agent string).

Each change is its own commit, so you can merge just the parts you want.

Static JavaScript assets (unless the admin changes settings)
get 0:30–24:00 hour freshness with async revalidating.

The related API responses get 2–58 second freshness, with caches
retaining stale data for up to 5 min on server errors.
HTTP request headers that influence the response needs to go in
the Vary response header.
Replaces time request argument containing the client's current
time with tz containing their time zone offset. Better for caching.

Properly handles time zone-adjusting dates and time. Times are no
longer off by up to a minute.
@da2x da2x changed the title Enable HTTP caching (performance) Enable HTTP caching (adds Cache-Control and Vary headers, changes some APIs from POST to GET, replaces time= with tz=) Dec 5, 2021
@da2x
Copy link
Contributor Author

da2x commented Dec 5, 2021

I’m done with this now. The main comment loading is still not cachable. I’ll handle that in a separate package after this is merged.

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

Successfully merging this pull request may close these issues.

1 participant