From 3d71b68195375f1b584bfe57904444b9cca6df12 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 13 Mar 2020 15:30:31 +0000 Subject: [PATCH] DEV: Introduce plugin api for conditionally rendering assets (#9200) --- .../common/_discourse_stylesheet.html.erb | 2 +- app/views/layouts/application.html.erb | 2 +- lib/discourse.rb | 14 ++++++++++-- lib/plugin/instance.rb | 9 ++++++++ plugins/discourse-internet-explorer/plugin.rb | 19 +++------------- spec/components/discourse_spec.rb | 22 +++++++++++++++++-- 6 files changed, 46 insertions(+), 22 deletions(-) diff --git a/app/views/common/_discourse_stylesheet.html.erb b/app/views/common/_discourse_stylesheet.html.erb index b6f64114e70..2312fa4da77 100644 --- a/app/views/common/_discourse_stylesheet.html.erb +++ b/app/views/common/_discourse_stylesheet.html.erb @@ -12,6 +12,6 @@ <%= discourse_stylesheet_link_tag(mobile_view? ? :mobile_theme : :desktop_theme) %> <%- end %> -<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?).each do |file| %> +<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request).each do |file| %> <%= discourse_stylesheet_link_tag(file) %> <%- end %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 3340017d75c..6c1f3b3074a 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -28,7 +28,7 @@ <%= preload_script "vendor" %> <%= preload_script "pretty-text-bundle" %> <%= preload_script "application" %> - <%- Discourse.find_plugin_js_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?).each do |file| %> + <%- Discourse.find_plugin_js_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, request: request).each do |file| %> <%= preload_script file %> <%- end %> <%- if staff? %> diff --git a/lib/discourse.rb b/lib/discourse.rb index 0cbc4d8e4cf..07810010b69 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -271,6 +271,10 @@ module Discourse def self.find_plugin_css_assets(args) plugins = self.find_plugins(args) + plugins = plugins.select do |plugin| + plugin.asset_filters.all? { |b| b.call(:css, args[:request]) } + end + assets = [] targets = [nil] @@ -289,9 +293,15 @@ module Discourse end def self.find_plugin_js_assets(args) - self.find_plugins(args).find_all do |plugin| + plugins = self.find_plugins(args).select do |plugin| plugin.js_asset_exists? - end.map { |plugin| "plugins/#{plugin.directory_name}" } + end + + plugins = plugins.select do |plugin| + plugin.asset_filters.all? { |b| b.call(:js, args[:request]) } + end + + plugins.map { |plugin| "plugins/#{plugin.directory_name}" } end def self.assets_digest diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index eb7c2322fc2..06070d5f02d 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -49,6 +49,7 @@ class Plugin::Instance :styles, :themes, :csp_extensions, + :asset_filters ].each do |att| class_eval %Q{ def #{att} @@ -413,6 +414,14 @@ class Plugin::Instance csp_extensions << extension end + # Register a block to run when adding css and js assets + # Two arguments will be passed: (type, request) + # Type is :css or :js. `request` is an instance of Rack::Request + # When using this, make sure to consider the effect on AnonymousCache + def register_asset_filter(&blk) + asset_filters << blk + end + # @option opts [String] :name # @option opts [String] :nativeName # @option opts [String] :fallbackLocale diff --git a/plugins/discourse-internet-explorer/plugin.rb b/plugins/discourse-internet-explorer/plugin.rb index eedd870fbdd..646dfff08a3 100644 --- a/plugins/discourse-internet-explorer/plugin.rb +++ b/plugins/discourse-internet-explorer/plugin.rb @@ -20,22 +20,9 @@ DiscourseEvent.on(:after_plugin_activation) do || end after_initialize do - - # Conditionally load the stylesheet. There is unfortunately no easy way to do this via - # Plugin API. - reloadable_patch do |plugin| - ApplicationHelper.module_eval do - alias_method :previous_discourse_stylesheet_link_tag, :discourse_stylesheet_link_tag - def discourse_stylesheet_link_tag(name, opts = {}) - - if name == 'discourse-internet-explorer' - return unless SiteSetting.discourse_internet_explorer_enabled? - return unless request.env['HTTP_USER_AGENT'] =~ /MSIE|Trident/ - end - - previous_discourse_stylesheet_link_tag(name, opts) - end - end + # Conditionally load the stylesheet + register_asset_filter do |type, request| + request.nil? || request.env['HTTP_USER_AGENT'] =~ /MSIE|Trident/ end register_anonymous_cache_key(:ie) do diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index 38625ee86d7..3f1e62d446f 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -75,12 +75,17 @@ describe Discourse do end end - let(:plugin1) { plugin_class.new.tap { |p| p.enabled = true } } - let(:plugin2) { plugin_class.new.tap { |p| p.enabled = false } } + let(:plugin1) { plugin_class.new.tap { |p| p.enabled = true; p.path = "my-plugin-1" } } + let(:plugin2) { plugin_class.new.tap { |p| p.enabled = false; p.path = "my-plugin-1" } } before { Discourse.plugins.append(plugin1, plugin2) } after { Discourse.plugins.clear } + before do + plugin_class.any_instance.stubs(:css_asset_exists?).returns(true) + plugin_class.any_instance.stubs(:js_asset_exists?).returns(true) + end + it 'can find plugins correctly' do expect(Discourse.plugins).to contain_exactly(plugin1, plugin2) @@ -90,6 +95,19 @@ describe Discourse do # Include disabled plugins when requested expect(Discourse.find_plugins(include_disabled: true)).to contain_exactly(plugin1, plugin2) end + + it 'can find plugin assets' do + plugin2.enabled = true + + expect(Discourse.find_plugin_css_assets({}).length).to eq(2) + expect(Discourse.find_plugin_js_assets({}).length).to eq(2) + plugin1.register_asset_filter do |type, request| + false + end + expect(Discourse.find_plugin_css_assets({}).length).to eq(1) + expect(Discourse.find_plugin_js_assets({}).length).to eq(1) + end + end context 'authenticators' do