-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Implement http_cache_forever to ActionController #18394
Conversation
Lovely! Need a CHANGELOG entry as well. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the significance of this date (curious)? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?