Implement http_cache_forever to ActionController #18394
Conversation
|
Lovely! Need a CHANGELOG entry as well. |
arthurnn
force-pushed
the
http_cache_forever
branch
2 times, most recently
from
33896c8
to
f8ad5c8
Compare
|
@dhh Changelog ✔️ |
| @@ -46,6 +46,20 @@ def cache_configured? | |||
| end | |||
| end | |||
|
|
|||
| # Cache or yield the block. The cache is suppose to never expire. | |||
There was a problem hiding this comment.
This needs to communicate why you'd reach for this method and what it does. You have an HTTP response that never changes, so you want browsers and proxies to cache it indefinitely. On the first hit, we serve the response with long-lived cache freshness response headers. Then subsequent requests will always revalidate as fresh.
This method belongs elsewhere, too. This module is concerned with caching within the app (fragments, template dependencies, etc) not HTTP cache freshness.
There was a problem hiding this comment.
Maybe should I move this to https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/conditional_get.rb ?
|
@jeremy I addressed most of your concerns, just leaving the where this should be still for decision. Let me know your thoughts. thanks |
| expires_in 100.years, public: public | ||
|
|
||
| yield if stale?(etag: "#{version}-#{request.fullpath}", | ||
| last_modified: Time.parse('2011-01-01').utc, |
There was a problem hiding this comment.
What was the significance of this date (curious)? Thanks.
There was a problem hiding this comment.
this is just a date on the past. But now I wonder if the last_modifed is necessary at all. I guess we could use only the etag, as that one has the version. Not 100% sure tho, as this is the opposite what @jeremy said in the comment above.. lets wait for him on this.
|
@arthurnn I worked on a PR but then I realized yours was much further along. I fetched yours, ran the tests, and read through them. Looks good! 👍 |
|
@jeremy Can you give this a final look over? Seems like we're just about ready for a merge. |
|
@jeremy let me know your thoughts, I can fix/rebase/merge this when you fell this is ready! |
|
🚢 |
|
@arthurnn just a reminder that you're also working on this and need to rebase 😄 |
|
merging this today, after breakfast =) |
Add http_cache_forever to ActionController, so we can cache results forever. Things like static pages are a good candidate for this type of caching. This cache only controls caching headers, so it is up to the browser to cache those requests.
arthurnn
force-pushed
the
http_cache_forever
branch
from
1b4e001
to
2ed3942
Compare
Implement http_cache_forever to ActionController
|
forever ❤️ |
|
🎉 😸 |
|
Boom! 🤘 |
Add http_cache_forever to ActionController, so we can cache results
forever.
Things like static pages are a good candidate for this type of caching.
This cache only controls caching headers, so it is up to the browser to
cache those requests.
First stab at #18192
@dhh @rafaelfranca @sgrif thoughts?