PERF: Improve `Accept` header handling for stylesheets and theme-js (#19357)
The default behavior for Rails is to vary the response of an endpoint based on the `Accept:` header, and therefore it returns a `Vary:` header on responses. This instructs browsers and intermediate proxies to key their caches based on the value of the request's `Accept` header. In some cases (e.g. Akamai), the presence of a `Vary` header is enough to prevent caching entirely. This commit restructures the Rails route definitions so that: 1. The "format" segment of the route is 'required' 2. The "format" segment of the route is constrained to a single value (e.g. `js` or `css`) Now that the routes are guaranteed to have a `:format` segment, Rails will always prioritize that over the `Accept` header, and will therefore omit the `Vary` header. Request specs are also added to test this behaviour for both stylesheets and theme-javascripts.
This commit is contained in:
parent
fc22790405
commit
1db3a578e4
|
@ -561,11 +561,11 @@ Discourse::Application.routes.draw do
|
|||
|
||||
get "highlight-js/:hostname/:version.js" => "highlight_js#show", constraints: { hostname: /[\w\.-]+/, format: :js }
|
||||
|
||||
get "stylesheets/:name.css.map" => "stylesheets#show_source_map", constraints: { name: /[-a-z0-9_]+/ }
|
||||
get "stylesheets/:name.css" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/ }
|
||||
get "stylesheets/:name" => "stylesheets#show_source_map", constraints: { name: /[-a-z0-9_]+/, format: /css\.map/ }, format: true
|
||||
get "stylesheets/:name" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/, format: "css" }, format: true
|
||||
get "color-scheme-stylesheet/:id(/:theme_id)" => "stylesheets#color_scheme", constraints: { format: :json }
|
||||
get "theme-javascripts/:digest.js" => "theme_javascripts#show", constraints: { digest: /\h{40}/ }
|
||||
get "theme-javascripts/:digest.map" => "theme_javascripts#show_map", constraints: { digest: /\h{40}/ }
|
||||
get "theme-javascripts/:digest" => "theme_javascripts#show", constraints: { digest: /\h{40}/, format: :js }, format: true
|
||||
get "theme-javascripts/:digest" => "theme_javascripts#show_map", constraints: { digest: /\h{40}/, format: :map }, format: true
|
||||
get "theme-javascripts/tests/:theme_id-:digest.js" => "theme_javascripts#show_tests"
|
||||
|
||||
post "uploads/lookup-metadata" => "uploads#metadata"
|
||||
|
|
|
@ -59,6 +59,30 @@ RSpec.describe StylesheetsController do
|
|||
expect(response.status).to eq(200)
|
||||
end
|
||||
|
||||
it 'ignores Accept header and does not include Vary header' do
|
||||
StylesheetCache.destroy_all
|
||||
manager = Stylesheet::Manager.new(theme_id: nil)
|
||||
builder = Stylesheet::Manager::Builder.new(target: 'desktop', manager: manager, theme: nil)
|
||||
builder.compile
|
||||
|
||||
digest = StylesheetCache.first.digest
|
||||
|
||||
get "/stylesheets/desktop_#{digest}.css"
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/css")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
|
||||
get "/stylesheets/desktop_#{digest}.css", headers: { "Accept" => "text/html" }
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/css")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
|
||||
get "/stylesheets/desktop_#{digest}.css", headers: { "Accept" => "invalidcontenttype" }
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/css")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
end
|
||||
|
||||
describe "#color_scheme" do
|
||||
it 'works as expected' do
|
||||
scheme = ColorScheme.last
|
||||
|
|
|
@ -75,6 +75,25 @@ RSpec.describe ThemeJavascriptsController do
|
|||
//# sourceMappingURL=#{digest}.map?__ws=test.localhost
|
||||
JS
|
||||
end
|
||||
|
||||
it "ignores Accept header and does not return Vary header" do
|
||||
js_cache_url = "/theme-javascripts/#{javascript_cache.digest}.js"
|
||||
|
||||
get js_cache_url
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/javascript")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
|
||||
get js_cache_url, headers: { "Accept" => "text/html" }
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/javascript")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
|
||||
get js_cache_url, headers: { "Accept" => "invalidcontenttype" }
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.headers["Content-Type"]).to eq("text/javascript")
|
||||
expect(response.headers["Vary"]).to eq(nil)
|
||||
end
|
||||
end
|
||||
|
||||
describe "#show_map" do
|
||||
|
|
Loading…
Reference in New Issue