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.

Sign up for GitHub

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

Jump to bottom

ability to specify cache-control header in config #124

Closed
wants to merge 1 commit into from
Closed

ability to specify cache-control header in config #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/propshaft/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ class Railtie < ::Rails::Railtie
[ "text/css", Propshaft::Compilers::SourceMappingUrls ],
[ "text/javascript", Propshaft::Compilers::SourceMappingUrls ]
]
if Rails.env.development?
config.assets.cache_control_header = "no-store"
else
config.assets.cache_control_header = "public, max-age=31536000, immutable"
Copy link

Choose a reason for hiding this comment

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

Does it make sense to have this value out-of-sync with config.assets.sweep_cache?
Wouldn't be easier to keep it in sync by default like

config.assets.cache_control_header = config.assets.sweep_cache ? "no-store" : "public, max-age=31536000, immutable"

❓

Copy link
Author

Choose a reason for hiding this comment

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

yep sweep_cache makes more sense 👍, I will change it later this week 😇

Copy link
Author

Choose a reason for hiding this comment

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

@simi on second thought this may be a bad idea

by relying on value of config.assets.sweep_cache we may end up overiding the value config.assets.cache_control_header if user wants to set it himself/herself:

Example:

# config/evvironments/development.rb
#...
config.assets.cache_control_header = "whatever"
config.assets.sweep_cache = true 
#...

...now what ?? Should the cache_control_header be no-store or whatever ?

I would say the original PR is least prone to edgecases = let the developer decide what value to return as cache_control_header

Please watch full explanation on loom:
Screenshot 2023-02-23 at 16 10 10

Copy link

@simi simi Feb 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Clear, this could be fixed by setting original value of sweep_cache to nil by default and only when it is not assigned to false explicitly, change cache_control_header and also self assign to false. Anything in here works for me. Even two independent config options as originally proposed. With sane default values per environment it would be rarely tweaked by users anyway.

Copy link
Author

@equivalent equivalent Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@simi Sorry I'm getting back after 2 weeks (I had some personal issues to solve)

Thank you for the recommendation how to fix this with nil sweep_cache. I understand

Even two independent config options as originally proposed

Unless you really want the sweep_cache nil solution (as you proposed) I would vote for the original "two independent config options" solution. I get it no-one will really change the values. But personally purely to have simple straight forward config.

please let me know if I should do the nil change proposed or not

Copy link

Choose a reason for hiding this comment

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

As mentioned no huge preference from my side. Let's grab some maintainers attention somehow, they can decide on their own. 🙏

@brenogazzola @dhh friendly ping

end
config.assets.sweep_cache = Rails.env.development?
config.assets.server = Rails.env.development? || Rails.env.test?

Expand Down
2 changes: 1 addition & 1 deletion lib/propshaft/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def call(env)
"content-type" => asset.content_type.to_s,
"accept-encoding" => "vary",
"etag" => asset.digest,
"cache-control" => "public, max-age=31536000, immutable"
"cache-control" => Rails.application.config.assets.cache_control_header
},
[ compiled_content ]
]
Expand Down