From 2d1dbc6f9616ce18807d87c8700c41ee73daa450 Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Fri, 7 Oct 2022 13:19:50 -0300 Subject: [PATCH] FEATURE: Preload resources via link header (#18475) Experiment moving from preload tags in the document head to preload information the the response headers. While this is a minor improvement in most browsers (headers are parsed before the response body), this allows smart proxies like Cloudflare to "learn" from those headers and build HTTP 103 Early Hints for subsequent requests to the same URI, which will allow the user agent to download and parse our JS/CSS while we are waiting for the server to generate and stream the HTML response. Co-authored-by: Penar Musaraj --- app/controllers/application_controller.rb | 13 ++++- app/helpers/application_helper.rb | 12 +++-- app/views/layouts/application.html.erb | 4 +- lib/stylesheet/manager.rb | 6 ++- spec/helpers/application_helper_spec.rb | 56 +++++++++++++++++--- spec/lib/stylesheet/manager_spec.rb | 21 ++++++++ spec/requests/application_controller_spec.rb | 12 +++++ 7 files changed, 107 insertions(+), 17 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d7bd7429229..60e23361be3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -49,7 +49,8 @@ class ApplicationController < ActionController::Base after_action :conditionally_allow_site_embedding after_action :ensure_vary_header after_action :add_noindex_header, if: -> { is_feed_request? || !SiteSetting.allow_index_in_robots_txt } - after_action :add_noindex_header_to_non_canonical, if: -> { request.get? && !(request.format && request.format.json?) && !request.xhr? } + after_action :add_noindex_header_to_non_canonical, if: :spa_boot_request? + around_action :link_preload, if: :spa_boot_request? HONEYPOT_KEY ||= 'HONEYPOT_KEY' CHALLENGE_KEY ||= 'CHALLENGE_KEY' @@ -1008,4 +1009,14 @@ class ApplicationController < ActionController::Base result end + + def link_preload + @links_to_preload = [] + yield + response.headers['Link'] = @links_to_preload.join(', ') if !@links_to_preload.empty? + end + + def spa_boot_request? + request.get? && !(request.format && request.format.json?) && !request.xhr? + end end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 0ee00f928ca..d667a5f2c4c 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -142,12 +142,16 @@ module ApplicationHelper end def preload_script_url(url) + add_resource_preload_list(url, 'script') <<~HTML.html_safe - HTML end + def add_resource_preload_list(resource_url, type) + @links_to_preload << %Q(<#{resource_url}>; rel="preload"; as="#{type}") if !@links_to_preload.nil? + end + def discourse_csrf_tags # anon can not have a CSRF token cause these are all pages # that may be cached, causing a mismatch between session CSRF @@ -589,7 +593,7 @@ module ApplicationHelper stylesheet_manager end - manager.stylesheet_link_tag(name, 'all') + manager.stylesheet_link_tag(name, 'all', self.method(:add_resource_preload_list)) end def discourse_preload_color_scheme_stylesheets @@ -605,10 +609,10 @@ module ApplicationHelper def discourse_color_scheme_stylesheets result = +"" - result << stylesheet_manager.color_scheme_stylesheet_link_tag(scheme_id, 'all') + result << stylesheet_manager.color_scheme_stylesheet_link_tag(scheme_id, 'all', self.method(:add_resource_preload_list)) if dark_scheme_id != -1 - result << stylesheet_manager.color_scheme_stylesheet_link_tag(dark_scheme_id, '(prefers-color-scheme: dark)') + result << stylesheet_manager.color_scheme_stylesheet_link_tag(dark_scheme_id, '(prefers-color-scheme: dark)', self.method(:add_resource_preload_list)) end result.html_safe diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 7db635e80d1..7fa63d6b27e 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -21,8 +21,8 @@ <%= build_plugin_html 'server:before-script-load' %> - " as="script"> - " as="script"> + <% add_resource_preload_list(script_asset_path("start-discourse"), "script") %> + <% add_resource_preload_list(script_asset_path("browser-update"), "script") %> <%= preload_script 'browser-detect' %> <%= preload_script "locales/#{I18n.locale}" %> diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index 72d4424d0c9..1ee4e8e5579 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -196,10 +196,11 @@ class Stylesheet::Manager end.join("\n").html_safe end - def stylesheet_link_tag(target = :desktop, media = 'all') + def stylesheet_link_tag(target = :desktop, media = 'all', preload_callback = nil) stylesheets = stylesheet_details(target, media) stylesheets.map do |stylesheet| href = stylesheet[:new_href] + preload_callback.call(href, 'style') if preload_callback theme_id = stylesheet[:theme_id] data_theme_id = theme_id ? "data-theme-id=\"#{theme_id}\"" : "" theme_name = stylesheet[:theme_name] @@ -311,12 +312,13 @@ class Stylesheet::Manager %[].html_safe end - def color_scheme_stylesheet_link_tag(color_scheme_id = nil, media = 'all') + def color_scheme_stylesheet_link_tag(color_scheme_id = nil, media = 'all', preload_callback = nil) stylesheet = color_scheme_stylesheet_details(color_scheme_id, media) return '' if !stylesheet href = stylesheet[:new_href] + preload_callback.call(href, 'style') if preload_callback css_class = media == 'all' ? "light-scheme" : "dark-scheme" diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index ee2f1cdc97f..102f8d7c323 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -3,9 +3,8 @@ RSpec.describe ApplicationHelper do describe "preload_script" do - def preload_link(url) + def script_tag(url) <<~HTML - HTML end @@ -32,7 +31,7 @@ RSpec.describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('discourse') - expect(link).to eq(preload_link("https://awesome.com/brotli_asset/discourse.js")) + expect(link).to eq(script_tag("https://awesome.com/brotli_asset/discourse.js")) end context "with s3 CDN" do @@ -61,36 +60,77 @@ RSpec.describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('discourse') - expect(link).to eq(preload_link("https://s3cdn.com/assets/discourse.br.js")) + expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse.br.js")) end it "gives s3 cdn if asset host is not set" do link = helper.preload_script('discourse') - expect(link).to eq(preload_link("https://s3cdn.com/assets/discourse.js")) + expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse.js")) end it "can fall back to gzip compression" do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'gzip' link = helper.preload_script('discourse') - expect(link).to eq(preload_link("https://s3cdn.com/assets/discourse.gz.js")) + expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse.gz.js")) end it "gives s3 cdn even if asset host is set" do set_cdn_url "https://awesome.com" link = helper.preload_script('discourse') - expect(link).to eq(preload_link("https://s3cdn.com/assets/discourse.js")) + expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse.js")) end it "gives s3 cdn but without brotli/gzip extensions for theme tests assets" do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'gzip, br' link = helper.preload_script('discourse/tests/theme_qunit_ember_jquery') - expect(link).to eq(preload_link("https://s3cdn.com/assets/discourse/tests/theme_qunit_ember_jquery.js")) + expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse/tests/theme_qunit_ember_jquery.js")) end end end + describe "add_resource_preload_list" do + it "adds resources to the preload list when it's available" do + @links_to_preload = [] + add_resource_preload_list('/assets/discourse.js', 'script') + add_resource_preload_list('/assets/discourse.css', 'style') + + expect(@links_to_preload.size).to eq(2) + end + + it "doesn't add resources to the preload list when it's not available" do + @links_to_preload = nil + add_resource_preload_list('/assets/discourse.js', 'script') + add_resource_preload_list('/assets/discourse.css', 'style') + + expect(@links_to_preload).to eq(nil) + end + + it "adds resources to the preload list when preload_script is called" do + @links_to_preload = [] + helper.preload_script('discourse') + + expect(@links_to_preload.size).to eq(1) + end + + it "adds resources to the preload list when discourse_stylesheet_link_tag is called" do + @links_to_preload = [] + helper.discourse_stylesheet_link_tag(:desktop) + + expect(@links_to_preload.size).to eq(1) + end + + it "adds resources as the correct type" do + @links_to_preload = [] + helper.discourse_stylesheet_link_tag(:desktop) + helper.preload_script('discourse') + + expect(@links_to_preload[0]).to match(/as="style"/) + expect(@links_to_preload[1]).to match(/as="script"/) + end + end + describe "escape_unicode" do it "encodes tags" do expect(helper.escape_unicode("")).to eq("\u003ctag>") diff --git a/spec/lib/stylesheet/manager_spec.rb b/spec/lib/stylesheet/manager_spec.rb index 218f2c49381..caae0e9a830 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/spec/lib/stylesheet/manager_spec.rb @@ -148,6 +148,16 @@ RSpec.describe Stylesheet::Manager do }) end + it "stylesheet_link_tag calls the preload callback when set" do + preload_list = [] + preload_callback = ->(href, type) { preload_list << [href, type] } + + manager = manager(theme.id) + expect { + manager.stylesheet_link_tag(:desktop_theme, 'all', preload_callback) + }.to change(preload_list, :size) + end + context "with stylesheet order" do let(:z_child_theme) do Fabricate(:theme, component: true, name: "ze component").tap do |z| @@ -638,6 +648,17 @@ RSpec.describe Stylesheet::Manager do expect(details1[:new_href]).not_to eq(details2[:new_href]) end + it "calls the preload callback when set" do + preload_list = [] + cs = Fabricate(:color_scheme, name: 'Funky') + theme = Fabricate(:theme, color_scheme_id: cs.id) + preload_callback = ->(href, type) { preload_list << [href, type] } + + expect { + manager.color_scheme_stylesheet_link_tag(theme.id, 'all', preload_callback) + }.to change(preload_list, :size).by(1) + end + context "with theme colors" do let(:theme) { Fabricate(:theme).tap { |t| t.set_field(target: :common, name: "color_definitions", value: ':root {--special: rebeccapurple;}') diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index d25d1a983d6..1828b1898c8 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -1124,4 +1124,16 @@ RSpec.describe ApplicationController do end end end + + describe 'preload Link header' do + it "should have the Link header with assets on full page requests" do + get("/latest") + expect(response.headers).to include('Link') + end + + it "shouldn't have the Link header on xhr api requests" do + get("/latest.json") + expect(response.headers).not_to include('Link') + end + end end