DEV: Debundle plugin javascript assets and don't load if disabled (#7566)

And don't load javascript assets if plugin is disabled.

* precompile auto generated plugin js assets

* SPEC: remove spec test functions

* remove plugin js from test_helper

Co-Authored-By: Régis Hanol <regis@hanol.fr>

* DEV: using equality is slightly easier to read than inequality

Co-Authored-By: Régis Hanol <regis@hanol.fr>

* DEV: use `select` method instead of `find_all` for readability

Co-Authored-By: Régis Hanol <regis@hanol.fr>

.gitignore

@@ -127,3 +127,6 @@ vendor/bundle/*
 
 # Vagrant
 .vagrant
+
+# ignore auto-generated plugin js assets
+/app/assets/javascripts/plugins/*

app/assets/javascripts/plugin-third-party.js.erb

@@ -1,15 +0,0 @@
-<%
-# Include plugin javascripts/handlebars templates
-Discourse.unofficial_plugins.each do |plugin|
-  plugin.javascript_includes.each { |js| require_asset(js) }
-  plugin.handlebars_includes.each { |hb| require_asset(hb) }
-
-  plugin.each_globbed_asset do |f, is_dir|
-    if is_dir
-      depend_on(f)
-    else
-      require_asset(f)
-    end
-  end
-end
-%>

app/assets/javascripts/plugin.js.erb

@@ -1,16 +0,0 @@
-<%
-# Include plugin javascripts/handlebars templates
-Discourse.official_plugins.each do |plugin|
-  plugin.javascript_includes.each { |js| require_asset(js) }
-  plugin.handlebars_includes.each { |hb| require_asset(hb) }
-
-  plugin.each_globbed_asset do |f, is_dir|
-    if is_dir
-      depend_on(f)
-    else
-      require_asset(f)
-    end
-  end
-end
-
-%>

app/views/layouts/application.html.erb

@@ -25,13 +25,9 @@
     <%= preload_script "vendor" %>
     <%= preload_script "pretty-text-bundle" %>
     <%= preload_script "application" %>
-    <%- if allow_plugins? %>
-    <%= preload_script "plugin" %>
+    <%- Discourse.find_plugin_js_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?).each do |file| %>
+      <%= preload_script file %>
     <%- end %>
-    <%- if allow_third_party_plugins? %>
-    <%= preload_script "plugin-third-party" %>
-    <%- end %>
-
     <%- if staff? %>
       <script src="<%= ExtraLocalesController.url('admin') %>"></script>
       <%= preload_script "admin" %>

config/application.rb

@@ -132,8 +132,6 @@ module Discourse
       pretty-text-bundle.js
       wizard-application.js
       wizard-vendor.js
-      plugin.js
-      plugin-third-party.js
       markdown-it-bundle.js
       service-worker.js
       google-tag-manager.js
@@ -261,6 +259,10 @@ module Discourse
       Discourse.activate_plugins!
     end
 
+    Discourse.find_plugin_js_assets(include_disabled: true).each do |file|
+      config.assets.precompile << "#{file}.js"
+    end
+
     require_dependency 'stylesheet/manager'
     require_dependency 'svg_sprite/svg_sprite'
 

lib/discourse.rb

@@ -204,6 +204,22 @@ module Discourse
     plugins.find_all { |p| !p.metadata.official? }
   end
 
+  def self.find_plugins(args)
+    plugins.select do |plugin|
+      next if args[:include_official] == false && plugin.metadata.official?
+      next if args[:include_unofficial] == false && !plugin.metadata.official?
+      next if args[:include_disabled] == false && !plugin.enabled?
+
+      true
+    end
+  end
+
+  def self.find_plugin_js_assets(args)
+    self.find_plugins(args).find_all do |plugin|
+      plugin.js_asset_exists?
+    end.map { |plugin| "plugins/#{plugin.asset_name}" }
+  end
+
   def self.assets_digest
     @assets_digest ||= begin
       digest = Digest::MD5.hexdigest(ActionView::Base.assets_manifest.assets.values.sort.join)

lib/plugin/instance.rb

@@ -529,6 +529,24 @@ class Plugin::Instance
       Discourse::Utils.execute_command('rm', '-f', target)
       Discourse::Utils.execute_command('ln', '-s', public_data, target)
     end
+
+    ensure_directory(Plugin::Instance.js_path)
+
+    contents = []
+    handlebars_includes.each { |hb| contents << "require_asset('#{hb}')" }
+    javascript_includes.each { |js| contents << "require_asset('#{js}')" }
+
+    each_globbed_asset do |f, is_dir|
+      contents << (is_dir ? "depend_on('#{f}')" : "require_asset('#{f}')")
+    end
+
+    File.delete(js_file_path) if js_asset_exists?
+
+    if contents.present?
+      contents.insert(0, "<%")
+      contents << "%>"
+      write_asset(js_file_path, contents.join("\n"))
+    end
   end
 
   def auth_provider(opts)
@@ -629,8 +647,24 @@ class Plugin::Instance
     end
   end
 
+  def asset_name
+    @asset_name ||= File.dirname(path).split("/").last
+  end
+
+  def js_asset_exists?
+    File.exists?(js_file_path)
+  end
+
   protected
 
+  def self.js_path
+    File.expand_path "#{Rails.root}/app/assets/javascripts/plugins"
+  end
+
+  def js_file_path
+    @file_path ||= "#{Plugin::Instance.js_path}/#{asset_name}.js.erb"
+  end
+
   def register_assets!
     assets.each do |asset, opts|
       DiscoursePluginRegistry.register_asset(asset, opts)

spec/requests/list_controller_spec.rb

@@ -646,20 +646,4 @@ RSpec.describe ListController do
       expect(topic_titles).to include(topic_in_sub_category.title)
     end
   end
-
-  describe "safe mode" do
-    it "handles safe mode" do
-      get "/latest"
-      expect(response.body).to match(/plugin\.js/)
-      expect(response.body).to match(/plugin-third-party\.js/)
-
-      get "/latest", params: { safe_mode: "no_plugins" }
-      expect(response.body).not_to match(/plugin\.js/)
-      expect(response.body).not_to match(/plugin-third-party\.js/)
-
-      get "/latest", params: { safe_mode: "only_official" }
-      expect(response.body).to match(/plugin\.js/)
-      expect(response.body).not_to match(/plugin-third-party\.js/)
-    end
-  end
 end

test/javascripts/test_helper.js

@@ -24,7 +24,6 @@
 //= require pretty-text-bundle
 //= require markdown-it-bundle
 //= require application
-//= require plugin
 //= require admin
 
 //= require sinon/pkg/sinon