From 95a57f7e0c7e78ecac14760ae4751e0ae2e8ea0d Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Fri, 7 Oct 2022 15:08:40 -0300 Subject: [PATCH] Revert "FEATURE: Preload resources via link header (#18475)" (#18511) This reverts commit 2d1dbc6f9616ce18807d87c8700c41ee73daa450. We need to increase nginx proxy buffer to land this. --- 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, 17 insertions(+), 107 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 60e23361be3..d7bd7429229 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -49,8 +49,7 @@ 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: :spa_boot_request? - around_action :link_preload, if: :spa_boot_request? + after_action :add_noindex_header_to_non_canonical, if: -> { request.get? && !(request.format && request.format.json?) && !request.xhr? } HONEYPOT_KEY ||= 'HONEYPOT_KEY' CHALLENGE_KEY ||= 'CHALLENGE_KEY' @@ -1009,14 +1008,4 @@ 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 d667a5f2c4c..0ee00f928ca 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -142,16 +142,12 @@ 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 @@ -593,7 +589,7 @@ module ApplicationHelper stylesheet_manager end - manager.stylesheet_link_tag(name, 'all', self.method(:add_resource_preload_list)) + manager.stylesheet_link_tag(name, 'all') end def discourse_preload_color_scheme_stylesheets @@ -609,10 +605,10 @@ module ApplicationHelper def discourse_color_scheme_stylesheets result = +"" - result << stylesheet_manager.color_scheme_stylesheet_link_tag(scheme_id, 'all', self.method(:add_resource_preload_list)) + result << stylesheet_manager.color_scheme_stylesheet_link_tag(scheme_id, 'all') if dark_scheme_id != -1 - result << stylesheet_manager.color_scheme_stylesheet_link_tag(dark_scheme_id, '(prefers-color-scheme: dark)', self.method(:add_resource_preload_list)) + result << stylesheet_manager.color_scheme_stylesheet_link_tag(dark_scheme_id, '(prefers-color-scheme: dark)') end result.html_safe diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 7fa63d6b27e..7db635e80d1 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' %> - <% add_resource_preload_list(script_asset_path("start-discourse"), "script") %> - <% add_resource_preload_list(script_asset_path("browser-update"), "script") %> + " as="script"> + " as="script"> <%= preload_script 'browser-detect' %> <%= preload_script "locales/#{I18n.locale}" %> diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index 1ee4e8e5579..72d4424d0c9 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -196,11 +196,10 @@ class Stylesheet::Manager end.join("\n").html_safe end - def stylesheet_link_tag(target = :desktop, media = 'all', preload_callback = nil) + def stylesheet_link_tag(target = :desktop, media = 'all') 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] @@ -312,13 +311,12 @@ class Stylesheet::Manager %[].html_safe end - def color_scheme_stylesheet_link_tag(color_scheme_id = nil, media = 'all', preload_callback = nil) + def color_scheme_stylesheet_link_tag(color_scheme_id = nil, media = 'all') 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 102f8d7c323..ee2f1cdc97f 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -3,8 +3,9 @@ RSpec.describe ApplicationHelper do describe "preload_script" do - def script_tag(url) + def preload_link(url) <<~HTML + HTML end @@ -31,7 +32,7 @@ RSpec.describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('discourse') - expect(link).to eq(script_tag("https://awesome.com/brotli_asset/discourse.js")) + expect(link).to eq(preload_link("https://awesome.com/brotli_asset/discourse.js")) end context "with s3 CDN" do @@ -60,77 +61,36 @@ RSpec.describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = 'br' link = helper.preload_script('discourse') - expect(link).to eq(script_tag("https://s3cdn.com/assets/discourse.br.js")) + expect(link).to eq(preload_link("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(script_tag("https://s3cdn.com/assets/discourse.js")) + expect(link).to eq(preload_link("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(script_tag("https://s3cdn.com/assets/discourse.gz.js")) + expect(link).to eq(preload_link("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(script_tag("https://s3cdn.com/assets/discourse.js")) + expect(link).to eq(preload_link("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(script_tag("https://s3cdn.com/assets/discourse/tests/theme_qunit_ember_jquery.js")) + expect(link).to eq(preload_link("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 caae0e9a830..218f2c49381 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/spec/lib/stylesheet/manager_spec.rb @@ -148,16 +148,6 @@ 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| @@ -648,17 +638,6 @@ 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 1828b1898c8..d25d1a983d6 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -1124,16 +1124,4 @@ 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