Revert "Revert "FEATURE: Preload resources via link header (#18475)" (#18511)" (#18531)

* Revert "Revert "FEATURE: Preload resources via link header (#18475)" (#18511)"

This reverts commit 95a57f7e0c.

* put behind feature flag

* env -> global setting

* declare global setting

* forgot one spot

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? && GlobalSetting.preload_link_header }
 
   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

app/helpers/application_helper.rb

@@ -142,10 +142,21 @@ module ApplicationHelper
   end
 
   def preload_script_url(url)
-    <<~HTML.html_safe
+    add_resource_preload_list(url, 'script')
-      <link rel="preload" href="#{url}" as="script">
+    if GlobalSetting.preload_link_header
-      <script defer src="#{url}"></script>
+      <<~HTML.html_safe
-    HTML
+        <script defer src="#{url}"></script>
+      HTML
+    else
+      <<~HTML.html_safe
+        <link rel="preload" href="#{url}" as="script">
+        <script defer src="#{url}"></script>
+      HTML
+    end
+  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
@@ -589,7 +600,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 +616,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

app/views/layouts/application.html.erb

@@ -7,7 +7,9 @@
     <meta name="discourse_theme_id" content="<%= theme_id %>">
     <meta name="discourse_current_homepage" content="<%= current_homepage %>">
 
-    <%= render partial: "common/discourse_preload_stylesheet" %>
+    <%- if GlobalSetting.preload_link_header %>
+      <%= render partial: "common/discourse_preload_stylesheet" %>
+    <%- end %>
     <%= render partial: "layouts/head" %>
     <%= discourse_csrf_tags %>
 
@@ -21,8 +23,13 @@
 
     <%= build_plugin_html 'server:before-script-load' %>
 
-    <link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script">
+    <%- if GlobalSetting.preload_link_header %>
-    <link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script">
+      <% add_resource_preload_list(script_asset_path("start-discourse"), "script") %>
+      <% add_resource_preload_list(script_asset_path("browser-update"), "script") %>
+    <%- else %>
+      <link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script">
+      <link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script">
+    <%- end %>  
     <%= preload_script 'browser-detect' %>
 
     <%= preload_script "locales/#{I18n.locale}" %>

config/discourse_defaults.conf

@@ -356,3 +356,6 @@ enable_long_polling =
 
 # Length of time to hold open a long polling connection in milliseconds
 long_polling_interval =
+
+# Moves asset preloading from tags in the response document head to response headers
+preload_link_header = false

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
     %[<link href="#{href}" rel="preload" as="style"/>].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"
 

spec/helpers/application_helper_spec.rb

@@ -3,7 +3,7 @@
 
 RSpec.describe ApplicationHelper do
   describe "preload_script" do
-    def preload_link(url)
+    def script_tag(url)
       <<~HTML
           <link rel="preload" href="#{url}" as="script">
           <script defer src="#{url}"></script>
@@ -32,7 +32,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 +61,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("<tag>")).to eq("\u003ctag>")

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;}')

spec/requests/application_controller_spec.rb

@@ -1124,4 +1124,38 @@ RSpec.describe ApplicationController do
       end
     end
   end
+
+  describe 'preload Link header' do
+    context "with GlobalSetting.preload_link_header" do
+      before do
+        global_setting :preload_link_header, true
+      end
+
+      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
+
+    context "without GlobalSetting.preload_link_header" do
+      before do
+        global_setting :preload_link_header, false
+      end
+
+      it "shouldn't have the Link header with assets on full page requests" do
+        get("/latest")
+        expect(response.headers).not_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
 end