From 1757a688c4749968db8e27623b975fc57519b97a Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 1 Feb 2024 11:48:31 +0000 Subject: [PATCH] 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 | 33 ------ lib/discourse_plugin_registry.rb | 30 +---- lib/plugin/instance.rb | 132 ++++++--------------- lib/pretty_text.rb | 24 ++-- spec/lib/discourse_plugin_registry_spec.rb | 14 --- spec/lib/plugin/instance_spec.rb | 8 +- 6 files changed, 47 insertions(+), 194 deletions(-) diff --git a/lib/discourse_js_processor.rb b/lib/discourse_js_processor.rb index 0526b0845e3..26d142fa4d8 100644 --- a/lib/discourse_js_processor.rb +++ b/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 diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index f018cf5faf1..e4bfcf0cd9b 100644 --- a/lib/discourse_plugin_registry.rb +++ b/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 - self.admin_javascripts << asset - elsif opts == :vendored_pretty_text + if 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 diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index eca1bd16cf7..3513f64a5cb 100644 --- a/lib/plugin/instance.rb +++ b/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) - - 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 + write_extra_js! 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 - "#{Plugin::Instance.js_path}/#{directory_name}.js.erb" + def legacy_asset_paths + [ + "#{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! diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index e22824a1179..e0a7083404f 100644 --- a/lib/pretty_text.rb +++ b/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 = [] - DiscoursePluginRegistry.each_globbed_asset do |a| - to_load << a if File.file?(a) && a =~ /discourse-markdown/ - end - to_load.uniq.each do |f| - plugin_name = f[%r{/plugins/([^/]+)/}, 1] - module_name = f[%r{/assets/javascripts/(.+)\.}, 1] - apply_es6_file( - ctx: ctx, - path: f, - module_name: "discourse/plugins/#{plugin_name}/#{module_name}", - ) + Discourse.plugins.each do |plugin| + Dir + .glob("#{plugin.directory}/assets/javascripts/**/discourse-markdown/**/*.js") + .filter { |a| File.file?(a) && a.end_with?(".js", ".js.es6") } + .each do |f| + module_name = + f.sub(%r{\A.+assets/javascripts/}, "discourse/plugins/#{plugin.name}/").sub( + /\.js(\.es6)?\z/, + "", + ) + 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)) } diff --git a/spec/lib/discourse_plugin_registry_spec.rb b/spec/lib/discourse_plugin_registry_spec.rb index 6bd872f78f0..f25b05cb195 100644 --- a/spec/lib/discourse_plugin_registry_spec.rb +++ b/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) diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb index 773e2f769c4..e2c0236b2b9 100644 --- a/spec/lib/plugin/instance_spec.rb +++ b/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.admin_javascripts.count).to eq(2) + expect(DiscoursePluginRegistry.javascripts.count).to eq(1) 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)