DEV: Remove sprockets from plugin 'extra js' pipeline (#25502)

JS assets added by plugins via `register_asset` will be outside the `assets/javascripts` directory, and are therefore exempt from being transpiled. That means that there isn't really any need to run them through DiscourseJsProcessor. Instead, we can just concatenate them together, and avoid the need for all the sprockets-wrangling.

This commit also takes the opportunity to clean up a number of plugin-asset-related codepaths which are no longer required (e.g. globs, handlebars)

lib/discourse_js_processor.rb

@@ -17,10 +17,6 @@ class DiscourseJsProcessor
     "proposal-export-namespace-from",
   ]
 
-  def self.plugin_transpile_paths
-    @@plugin_transpile_paths ||= Set.new
-  end
-
   def self.ember_cli?(filename)
     filename.include?("/app/assets/javascripts/discourse/dist/")
   end
@@ -33,19 +29,6 @@ class DiscourseJsProcessor
 
     data = transpile(data, root_path, logical_path) if should_transpile?(input[:filename])
 
-    # add sourceURL until we can do proper source maps
-    if !Rails.env.production? && !ember_cli?(input[:filename])
-      plugin_name = root_path[%r{/plugins/([\w-]+)/assets}, 1]
-      source_url =
-        if plugin_name
-          "plugins/#{plugin_name}/assets/javascripts/#{logical_path}"
-        else
-          logical_path
-        end
-
-      data = "eval(#{data.inspect} + \"\\n//# sourceURL=#{source_url}\");\n"
-    end
-
     { data: data }
   end
 
@@ -74,22 +57,6 @@ class DiscourseJsProcessor
     return false if relative_path.start_with?("#{js_root}/locales/")
     return false if relative_path.start_with?("#{js_root}/plugins/")
 
-    if %w[
-         start-discourse
-         onpopstate-handler
-         google-tag-manager
-         google-universal-analytics-v3
-         google-universal-analytics-v4
-         activate-account
-         auto-redirect
-         embed-application
-         app-boot
-       ].any? { |f| relative_path == "#{js_root}/#{f}.js" }
-      return true
-    end
-
-    return true if plugin_transpile_paths.any? { |prefix| relative_path.start_with?(prefix) }
-
     !!(relative_path =~ %r{^#{js_root}/[^/]+/} || relative_path =~ %r{^#{test_root}/[^/]+/})
   end
 

lib/discourse_plugin_registry.rb

@@ -53,18 +53,15 @@ class DiscoursePluginRegistry
   define_register :javascripts, Set
   define_register :auth_providers, Set
   define_register :service_workers, Set
-  define_register :admin_javascripts, Set
   define_register :stylesheets, Hash
   define_register :mobile_stylesheets, Hash
   define_register :desktop_stylesheets, Hash
   define_register :color_definition_stylesheets, Hash
-  define_register :handlebars, Set
   define_register :serialized_current_user_fields, Set
   define_register :seed_data, HashWithIndifferentAccess
   define_register :locales, HashWithIndifferentAccess
   define_register :svg_icons, Set
   define_register :custom_html, Hash
-  define_register :asset_globs, Set
   define_register :html_builders, Hash
   define_register :seed_path_builders, Set
   define_register :vendored_pretty_text, Set
@@ -158,34 +155,11 @@ class DiscoursePluginRegistry
     Archetype.register(name, options)
   end
 
-  def self.register_glob(root, extension, options = nil)
-    self.asset_globs << [root, extension, options || {}]
-  end
-
-  def self.each_globbed_asset(each_options = nil)
-    each_options ||= {}
-
-    self.asset_globs.each do |g|
-      root, ext, options = *g
-
-      if options[:admin]
-        next unless each_options[:admin]
-      else
-        next if each_options[:admin]
-      end
-
-      Dir.glob("#{root}/**/*.#{ext}") { |f| yield f }
-    end
-  end
-
   JS_REGEX = /\.js$|\.js\.erb$|\.js\.es6\z/
-  HANDLEBARS_REGEX = /\.(hb[rs]|js\.handlebars)\z/
 
   def self.register_asset(asset, opts = nil, plugin_directory_name = nil)
     if asset =~ JS_REGEX
-      if opts == :admin
+      if opts == :vendored_pretty_text
-        self.admin_javascripts << asset
-      elsif opts == :vendored_pretty_text
         self.vendored_pretty_text << asset
       elsif opts == :vendored_core_pretty_text
         self.vendored_core_pretty_text << asset
@@ -205,8 +179,6 @@ class DiscoursePluginRegistry
         self.stylesheets[plugin_directory_name] ||= Set.new
         self.stylesheets[plugin_directory_name] << asset
       end
-    elsif asset =~ HANDLEBARS_REGEX
-      self.handlebars << asset
     end
   end
 

lib/plugin/instance.rb

@@ -663,6 +663,11 @@ class Plugin::Instance
         Any hbs files under `assets/javascripts` will be automatically compiled and included."
       ERROR
 
+    raise <<~ERROR if file.start_with?("javascripts/") && file.end_with?(".js", ".js.es6")
+        [#{name}] Javascript files under `assets/javascripts` are automatically included in JS bundles.
+        Manual register_asset calls should be removed. (attempted to add #{file})
+      ERROR
+
     if opts && opts == :vendored_core_pretty_text
       full_path = DiscoursePluginRegistry.core_asset_for_name(file)
     else
@@ -719,38 +724,6 @@ class Plugin::Instance
   # this allows us to present information about a plugin in the UI
   # prior to activations
   def activate!
-    if @path
-      root_dir_name = File.dirname(@path)
-
-      # Automatically include all ES6 JS and hbs files
-      root_path = "#{root_dir_name}/assets/javascripts"
-      DiscoursePluginRegistry.register_glob(root_path, "js")
-      DiscoursePluginRegistry.register_glob(root_path, "js.es6")
-      DiscoursePluginRegistry.register_glob(root_path, "hbs")
-      DiscoursePluginRegistry.register_glob(root_path, "hbr")
-
-      admin_path = "#{root_dir_name}/admin/assets/javascripts"
-      DiscoursePluginRegistry.register_glob(admin_path, "js", admin: true)
-      DiscoursePluginRegistry.register_glob(admin_path, "js.es6", admin: true)
-      DiscoursePluginRegistry.register_glob(admin_path, "hbs", admin: true)
-      DiscoursePluginRegistry.register_glob(admin_path, "hbr", admin: true)
-
-      DiscourseJsProcessor.plugin_transpile_paths << root_path.sub(Rails.root.to_s, "").sub(
-        %r{\A/*},
-        "",
-      )
-      DiscourseJsProcessor.plugin_transpile_paths << admin_path.sub(Rails.root.to_s, "").sub(
-        %r{\A/*},
-        "",
-      )
-
-      test_path = "#{root_dir_name}/test/javascripts"
-      DiscourseJsProcessor.plugin_transpile_paths << test_path.sub(Rails.root.to_s, "").sub(
-        %r{\A/*},
-        "",
-      )
-    end
-
     self.instance_eval File.read(path), path
     if auto_assets = generate_automatic_assets!
       assets.concat(auto_assets)
@@ -762,14 +735,6 @@ class Plugin::Instance
 
     seed_data.each { |key, value| DiscoursePluginRegistry.register_seed_data(key, value) }
 
-    # TODO: possibly amend this to a rails engine
-
-    # Automatically include assets
-    Rails.configuration.assets.paths << auto_generated_path
-    Rails.configuration.assets.paths << File.dirname(path) + "/assets"
-    Rails.configuration.assets.paths << File.dirname(path) + "/admin/assets"
-    Rails.configuration.assets.paths << File.dirname(path) + "/test/javascripts"
-
     # Automatically include rake tasks
     Rake.add_rakelib(File.dirname(path) + "/lib/tasks")
 
@@ -791,29 +756,7 @@ class Plugin::Instance
       Discourse::Utils.atomic_ln_s(public_data, target)
     end
 
-    ensure_directory(js_file_path)
+    write_extra_js!
-
-    contents = []
-    handlebars_includes.each { |hb| contents << "require_asset('#{hb}')" }
-    javascript_includes.each { |js| contents << "require_asset('#{js}')" }
-
-    if !contents.present?
-      [js_file_path, extra_js_file_path].each do |f|
-        File.delete(f)
-      rescue Errno::ENOENT
-      end
-      return
-    end
-
-    contents.insert(0, "<%")
-    contents << "%>"
-
-    Discourse::Utils.atomic_write_file(extra_js_file_path, contents.join("\n"))
-
-    begin
-      File.delete(js_file_path)
-    rescue Errno::ENOENT
-    end
   end
 
   def auth_provider(opts)
@@ -869,49 +812,16 @@ class Plugin::Instance
     end
   end
 
-  def handlebars_includes
-    assets
-      .map do |asset, opts|
-        next if opts == :admin
-        next unless asset =~ DiscoursePluginRegistry::HANDLEBARS_REGEX
-        asset
-      end
-      .compact
-  end
-
   def javascript_includes
     assets
       .map do |asset, opts|
         next if opts == :vendored_core_pretty_text
-        next if opts == :admin
         next unless asset =~ DiscoursePluginRegistry::JS_REGEX
         asset
       end
       .compact
   end
 
-  def each_globbed_asset
-    if @path
-      # Automatically include all ES6 JS and hbs files
-      root_path = "#{File.dirname(@path)}/assets/javascripts"
-      admin_path = "#{File.dirname(@path)}/admin/assets/javascripts"
-
-      Dir
-        .glob(["#{root_path}/**/*", "#{admin_path}/**/*"])
-        .sort
-        .each do |f|
-          f_str = f.to_s
-          if File.directory?(f)
-            yield [f, true]
-          elsif f_str.end_with?(".js.es6") || f_str.end_with?(".hbs") || f_str.end_with?(".hbr")
-            yield [f, false]
-          elsif f_str.end_with?(".js")
-            yield [f, false]
-          end
-        end
-    end
-  end
-
   def register_reviewable_type(reviewable_type_class)
     extend_list_method Reviewable, :types, [reviewable_type_class.name]
   end
@@ -1296,12 +1206,36 @@ class Plugin::Instance
     File.expand_path "#{Rails.root}/app/assets/javascripts/plugins"
   end
 
-  def js_file_path
+  def legacy_asset_paths
-    "#{Plugin::Instance.js_path}/#{directory_name}.js.erb"
+    [
+      "#{Plugin::Instance.js_path}/#{directory_name}.js.erb",
+      "#{Plugin::Instance.js_path}/#{directory_name}_extra.js.erb",
+    ]
   end
 
   def extra_js_file_path
-    @extra_js_file_path ||= "#{Plugin::Instance.js_path}/#{directory_name}_extra.js.erb"
+    @extra_js_file_path ||= "#{Plugin::Instance.js_path}/#{directory_name}_extra.js"
+  end
+
+  def write_extra_js!
+    # No longer used, but we want to make sure the files are no longer present
+    # so they don't accidently get compiled by Sprockets.
+    legacy_asset_paths.each do |path|
+      File.delete(path)
+    rescue Errno::ENOENT
+    end
+
+    contents = javascript_includes.map { |js| File.read(js) }
+
+    if contents.present?
+      ensure_directory(extra_js_file_path)
+      Discourse::Utils.atomic_write_file(extra_js_file_path, contents.join("\n;\n"))
+    else
+      begin
+        File.delete(extra_js_file_path)
+      rescue Errno::ENOENT
+      end
+    end
   end
 
   def register_assets!

lib/pretty_text.rb

@@ -103,18 +103,18 @@ module PrettyText
     ctx.load("#{Rails.root}/lib/pretty_text/shims.js")
     ctx.eval("__setUnicode(#{Emoji.unicode_replacements_json})")
 
-    to_load = []
+    Discourse.plugins.each do |plugin|
-    DiscoursePluginRegistry.each_globbed_asset do |a|
+      Dir
-      to_load << a if File.file?(a) && a =~ /discourse-markdown/
+        .glob("#{plugin.directory}/assets/javascripts/**/discourse-markdown/**/*.js")
-    end
+        .filter { |a| File.file?(a) && a.end_with?(".js", ".js.es6") }
-    to_load.uniq.each do |f|
+        .each do |f|
-      plugin_name = f[%r{/plugins/([^/]+)/}, 1]
+          module_name =
-      module_name = f[%r{/assets/javascripts/(.+)\.}, 1]
+            f.sub(%r{\A.+assets/javascripts/}, "discourse/plugins/#{plugin.name}/").sub(
-      apply_es6_file(
+              /\.js(\.es6)?\z/,
-        ctx: ctx,
+              "",
-        path: f,
-        module_name: "discourse/plugins/#{plugin_name}/#{module_name}",
             )
+          apply_es6_file(ctx: ctx, path: f, module_name: module_name)
+        end
     end
 
     DiscoursePluginRegistry.vendored_core_pretty_text.each { |vpt| ctx.eval(File.read(vpt)) }

spec/lib/discourse_plugin_registry_spec.rb

@@ -83,13 +83,6 @@ RSpec.describe DiscoursePluginRegistry do
     end
   end
 
-  describe "#admin_javascripts" do
-    it "defaults to an empty Set" do
-      registry.reset!
-      expect(registry.admin_javascripts).to eq(Set.new)
-    end
-  end
-
   describe "#seed_data" do
     it "defaults to an empty Set" do
       registry.reset!
@@ -230,13 +223,6 @@ RSpec.describe DiscoursePluginRegistry do
       expect(registry.stylesheets[plugin_directory_name]).to eq(nil)
     end
 
-    it "registers admin javascript properly" do
-      registry.register_asset("my_admin.js", :admin)
-
-      expect(registry.admin_javascripts.count).to eq(1)
-      expect(registry.javascripts.count).to eq(0)
-    end
-
     it "registers vendored_core_pretty_text properly" do
       registry.register_asset("my_lib.js", :vendored_core_pretty_text)
 

spec/lib/plugin/instance_spec.rb

@@ -408,15 +408,9 @@ TEXT
       plugin.register_asset("desktop.css", :desktop)
       plugin.register_asset("desktop2.css", :desktop)
 
-      plugin.register_asset("code.js")
-
-      plugin.register_asset("my_admin.js", :admin)
-      plugin.register_asset("my_admin2.js", :admin)
-
       plugin.activate!
 
-      expect(DiscoursePluginRegistry.javascripts.count).to eq(2)
+      expect(DiscoursePluginRegistry.javascripts.count).to eq(1)
-      expect(DiscoursePluginRegistry.admin_javascripts.count).to eq(2)
       expect(DiscoursePluginRegistry.desktop_stylesheets[plugin.directory_name].count).to eq(2)
       expect(DiscoursePluginRegistry.stylesheets[plugin.directory_name].count).to eq(2)
       expect(DiscoursePluginRegistry.mobile_stylesheets[plugin.directory_name].count).to eq(1)