Skip to content

Commit

Permalink
DEV: Remove sprockets from plugin 'extra js' pipeline (discourse#25502)
Browse files Browse the repository at this point in the history 
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)
  • Loading branch information
@davidtaylorhq
davidtaylorhq authored Feb 1, 2024
1 parent 4c25266 commit 1757a68
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 194 deletions.
33 changes: 0 additions & 33 deletions lib/discourse_js_processor.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ class TranspileError < StandardError
"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
Expand All @@ -33,19 +29,6 @@ def self.call(input)

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

Expand Down Expand Up @@ -74,22 +57,6 @@ def self.should_transpile?(filename)
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

Expand Down
30 changes: 1 addition & 29 deletions lib/discourse_plugin_registry.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,15 @@ def self.define_filtered_register(register_name)
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
Expand Down Expand Up @@ -158,34 +155,11 @@ def register_archetype(name, options = {})
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
Expand All @@ -205,8 +179,6 @@ def self.register_asset(asset, opts = nil, plugin_directory_name = nil)
self.stylesheets[plugin_directory_name] ||= Set.new
self.stylesheets[plugin_directory_name] << asset
end
elsif asset =~ HANDLEBARS_REGEX
self.handlebars << asset
end
end

Expand Down
132 changes: 33 additions & 99 deletions lib/plugin/instance.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,11 @@ def register_asset(file, opts = nil)
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
Expand Down Expand Up @@ -719,38 +724,6 @@ def automatic_assets
# 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)
Expand All @@ -762,14 +735,6 @@ def activate!

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")

Expand All @@ -791,29 +756,7 @@ def activate!
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)
Expand Down Expand Up @@ -869,49 +812,16 @@ def enabled_site_setting(setting = nil)
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
Expand Down Expand Up @@ -1296,12 +1206,36 @@ def self.js_path
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!
Expand Down
24 changes: 12 additions & 12 deletions lib/pretty_text.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,18 @@ def self.create_es6_context
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)) }
Expand Down
14 changes: 0 additions & 14 deletions spec/lib/discourse_plugin_registry_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,6 @@ def enabled?
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!
Expand Down Expand Up @@ -230,13 +223,6 @@ def enabled?
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)

Expand Down
8 changes: 1 addition & 7 deletions spec/lib/plugin/instance_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,15 +408,9 @@ def enabled?
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)
Expand Down

0 comments on commit 1757a68

Please sign in to comment.