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

[Chore] Enable caching #11

Merged
merged 28 commits into from
Sep 6, 2022
Merged

Conversation

samkahchiin
Copy link
Contributor

@samkahchiin samkahchiin commented Jun 1, 2022

Summary

Customise caching in the cloudflare worker script.
This PR sets two Page Rules: Edge Cache TTL and Cache Level (to Cache Everything) using the request url as the cacheKey on the processed request

Edge Cache TTL VS Browser cache Expire TTL here
=> Browser cache TTL

Browser cache expire TTL is the time that CloudFlare instructs a visitor's browser to cache a resource. Until this time expires, the browser will load the resource from its local cache thus speeding up the request significantly. CloudFlare will respect the headers that you give us from your web server, and then we will communicate with the browser based on the time selected in this drop down menu.

=> Edge Cache TTL

how long CloudFlare's edge servers will cache a resource before requesting a fresh copy from your server. When you create a Cache Everything Page Rule, you now may choose whether to respect all existing headers or to override any headers that are in place from your server. By overwriting the headers, CloudFlare will cache more content at the CloudFlare edge network, decreasing load to your server.

Unfortunately, edge cache TTL would not be reflected in the response header. See here

Edge Cache TTL isn't visible in response headers.

Regarding html caching
The Cloudflare router by default does not cache html file by default

Cloudflare only caches based on file extension and not by MIME type. The Cloudflare CDN does not cache HTML by default

See here

Setting cacheEverything to true to allow html file to be cached by Cloudflare as well.
https://developers.cloudflare.com/workers/runtime-apis/request#requestinitcfproperties

This option forces Cloudflare to cache the response for this request, regardless of what headers are seen on the response. This is equivalent to setting the Page Rule Cache Level (to Cache Everything). Defaults to false. This option applies to GET and HEAD request methods only.

TODO

  • Should make this as optional choices as it might not work well in some cases
  • Setting cacheEverything to true can be dangerous for dynamic pages. Investigate how it affect the current setup
    • Solution: Limit cloudflare worker to only cache request which match the specified routes

References

@samkahchiin samkahchiin closed this Jun 1, 2022
@samkahchiin samkahchiin reopened this Jun 1, 2022
@samkahchiin samkahchiin marked this pull request as draft June 1, 2022 10:20
@samkahchiin samkahchiin marked this pull request as ready for review June 1, 2022 10:22
@samkahchiin samkahchiin marked this pull request as draft June 1, 2022 10:26
@samkahchiin samkahchiin force-pushed the chore/kahchiinsam/add-cdn-caches branch from 1a02525 to 29b1daf Compare August 29, 2022 11:53
@samkahchiin samkahchiin marked this pull request as ready for review August 30, 2022 06:15
@swiknaba swiknaba requested a review from puckzxz August 31, 2022 21:31
src/cloudflare-router.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@swiknaba swiknaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, thanks! Tested on our test application, loading some resources is broken now. See the comment from @puckzxz , that should fix it 🙂

@swiknaba swiknaba assigned samkahchiin and unassigned marcqualie Aug 31, 2022
@samkahchiin samkahchiin force-pushed the chore/kahchiinsam/add-cdn-caches branch from 5d2558e to a03d5f4 Compare September 1, 2022 09:13
@samkahchiin samkahchiin removed their assignment Sep 5, 2022
@samkahchiin samkahchiin added the enhancement New feature or request label Sep 5, 2022
package.json Outdated Show resolved Hide resolved
swiknaba
swiknaba previously approved these changes Sep 5, 2022
Copy link
Contributor

@swiknaba swiknaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution: Limit cloudflare worker to only cache request which match the specified routes

Makes sense!

Thanks!

Left a few minor clean up suggestions

Co-authored-by: Lud <swiknaba@users.noreply.github.com>
samkahchiin and others added 2 commits September 6, 2022 11:22
Co-authored-by: Lud <swiknaba@users.noreply.github.com>
Copy link
Contributor

@swiknaba swiknaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@swiknaba swiknaba assigned samkahchiin and unassigned swiknaba Sep 6, 2022
@samkahchiin samkahchiin merged commit d4a43a5 into main Sep 6, 2022
@samkahchiin samkahchiin deleted the chore/kahchiinsam/add-cdn-caches branch September 6, 2022 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants