From 52d833472cd6ce99141adf7f2363bf9cd2c53d61 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 12 Mar 2021 11:17:42 -0500 Subject: [PATCH] DEV: Refactor plugin SCSS compilation (#12359) --- lib/stylesheet/common.rb | 9 ----- lib/stylesheet/compiler.rb | 21 +++++----- lib/stylesheet/importer.rb | 38 ++----------------- spec/components/plugin/instance_spec.rb | 2 +- spec/components/stylesheet/compiler_spec.rb | 25 +++++++++--- .../assets/stylesheets/colors.scss | 2 + .../assets/stylesheets/common/_variables.scss | 1 + .../assets/stylesheets/common/common.scss | 13 +++++++ .../stylesheets/common/subfolder/other.scss | 6 +++ spec/fixtures/plugins/scss_plugin/plugin.rb | 7 ++++ spec/models/theme_field_spec.rb | 2 +- 11 files changed, 66 insertions(+), 60 deletions(-) delete mode 100644 lib/stylesheet/common.rb create mode 100644 spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss create mode 100644 spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/_variables.scss create mode 100644 spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss create mode 100644 spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/subfolder/other.scss create mode 100644 spec/fixtures/plugins/scss_plugin/plugin.rb diff --git a/lib/stylesheet/common.rb b/lib/stylesheet/common.rb deleted file mode 100644 index b64f9257fb6..00000000000 --- a/lib/stylesheet/common.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -require 'sassc' - -module Stylesheet - module Common - ASSET_ROOT = "#{Rails.root}/app/assets/stylesheets" unless defined? ASSET_ROOT - end -end diff --git a/lib/stylesheet/compiler.rb b/lib/stylesheet/compiler.rb index 8f9f0255183..efc47185774 100644 --- a/lib/stylesheet/compiler.rb +++ b/lib/stylesheet/compiler.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true -require 'stylesheet/common' require 'stylesheet/importer' require 'stylesheet/functions' module Stylesheet class Compiler + ASSET_ROOT = "#{Rails.root}/app/assets/stylesheets" unless defined? ASSET_ROOT def self.compile_asset(asset, options = {}) importer = Importer.new(options) @@ -16,12 +16,16 @@ module Stylesheet filename = "theme_#{options[:theme_id]}.scss" file += options[:theme_variables].to_s file += importer.theme_import(asset) - elsif Importer.special_imports[asset.to_s] - filename = "theme_#{options[:theme_id]}.scss" - file += " @import \"#{asset}\";" + elsif plugin_assets = Importer.plugin_assets[asset.to_s] + filename = "#{asset.to_s}.scss" + options[:load_paths] = [] if options[:load_paths].nil? + plugin_assets.each do |src| + file += File.read src + options[:load_paths] << File.expand_path(File.dirname(src)) + end else filename = "#{asset}.scss" - path = "#{Stylesheet::Common::ASSET_ROOT}/#{filename}" + path = "#{ASSET_ROOT}/#{filename}" file += File.read path case asset.to_s @@ -32,9 +36,7 @@ module Stylesheet file += importer.font when "wizard" file += importer.wizard_fonts - end - - if asset.to_s == Stylesheet::Manager::COLOR_SCHEME_STYLESHEET + when Stylesheet::Manager::COLOR_SCHEME_STYLESHEET file += importer.import_color_definitions file += importer.import_wcag_overrides end @@ -46,11 +48,10 @@ module Stylesheet def self.compile(stylesheet, filename, options = {}) source_map_file = options[:source_map_file] || "#{filename.sub(".scss", "")}.css.map" - load_paths = [Stylesheet::Common::ASSET_ROOT] + load_paths = [ASSET_ROOT] load_paths += options[:load_paths] if options[:load_paths] engine = SassC::Engine.new(stylesheet, - importer: Importer, filename: filename, style: :compressed, source_map_file: source_map_file, diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb index 750957e2e29..47f8e709a5d 100644 --- a/lib/stylesheet/importer.rb +++ b/lib/stylesheet/importer.rb @@ -1,26 +1,18 @@ # frozen_string_literal: true -require_dependency 'stylesheet/common' require_dependency 'global_path' module Stylesheet - class Importer < SassC::Importer + class Importer include GlobalPath THEME_TARGETS ||= %w{embedded_theme mobile_theme desktop_theme} - def self.special_imports - @special_imports ||= {} + def self.plugin_assets + @plugin_assets ||= {} end - def self.register_import(name, &blk) - special_imports[name] = blk - end - - # Contained in function so that it can be called repeatedly from test mode def self.register_imports! - @special_imports = {} - Discourse.plugins.each do |plugin| plugin_directory_name = plugin.directory_name @@ -28,11 +20,7 @@ module Stylesheet asset_name = type.present? ? "#{plugin_directory_name}_#{type}" : plugin_directory_name stylesheets = type.present? ? DiscoursePluginRegistry.send("#{type}_stylesheets") : DiscoursePluginRegistry.stylesheets - if stylesheets[plugin_directory_name].present? - register_import asset_name do - import_files(stylesheets[plugin_directory_name]) - end - end + plugin_assets[asset_name] = stylesheets[plugin_directory_name] if plugin_directory_name.present? end end @@ -173,17 +161,6 @@ module Stylesheet end end - def import_files(files) - files.map do |file| - # we never want inline css imports, they are a mess - # this tricks libsass so it imports inline instead - if file =~ /\.css$/ - file = file[0..-5] - end - Import.new(file) - end - end - def theme_import(target) attr = target == :embedded_theme ? :embedded_scss : :scss target = target.to_s.gsub("_theme", "").to_sym @@ -238,12 +215,5 @@ module Stylesheet contents end - def imports(asset, parent_path) - if callback = Importer.special_imports[asset] - instance_eval(&callback) - else - Import.new(asset + ".scss") - end - end end end diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb index 9c67ee8a036..e9de28e83d1 100644 --- a/spec/components/plugin/instance_spec.rb +++ b/spec/components/plugin/instance_spec.rb @@ -11,7 +11,7 @@ describe Plugin::Instance do context "find_all" do it "can find plugins correctly" do plugins = Plugin::Instance.find_all("#{Rails.root}/spec/fixtures/plugins") - expect(plugins.count).to eq(4) + expect(plugins.count).to eq(5) plugin = plugins[3] expect(plugin.name).to eq("plugin-name") diff --git a/spec/components/stylesheet/compiler_spec.rb b/spec/components/stylesheet/compiler_spec.rb index 08af16c2056..abbe6029167 100644 --- a/spec/components/stylesheet/compiler_spec.rb +++ b/spec/components/stylesheet/compiler_spec.rb @@ -29,11 +29,17 @@ describe Stylesheet::Compiler do context "with a plugin" do before do - plugin = Plugin::Instance.new - plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" - plugin.register_css "body { background: $primary }" - Discourse.plugins << plugin - plugin.activate! + plugin1 = Plugin::Instance.new + plugin1.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" + plugin1.register_css "body { background: $primary }" + + plugin2 = Plugin::Instance.new + plugin2.path = "#{Rails.root}/spec/fixtures/plugins/scss_plugin/plugin.rb" + + Discourse.plugins << plugin1 + Discourse.plugins << plugin2 + plugin1.activate! + plugin2.activate! Stylesheet::Importer.register_imports! end @@ -48,6 +54,15 @@ describe Stylesheet::Compiler do expect(css).not_to include(upload.url) expect(css).to include("background:") end + + it "supports SCSS imports" do + css, _map = Stylesheet::Compiler.compile_asset("scss_plugin", theme_id: theme.id) + + expect(css).to include("border-color:red") + expect(css).to include("fill:green") + expect(css).to include("line-height:1.2em") + expect(css).to include("border-color:#c00") + end end end diff --git a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss new file mode 100644 index 00000000000..c4b12651aa7 --- /dev/null +++ b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss @@ -0,0 +1,2 @@ +$color1: red; +$color2: green; diff --git a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/_variables.scss b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/_variables.scss new file mode 100644 index 00000000000..fb8f9f69fdb --- /dev/null +++ b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/_variables.scss @@ -0,0 +1 @@ +$lineheight: 1.2em; diff --git a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss new file mode 100644 index 00000000000..0ff1ecfff14 --- /dev/null +++ b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss @@ -0,0 +1,13 @@ +@import "_variables"; +@import "../colors"; +@import "subfolder/other"; + +body { + border-color: $color1; + fill: $color2; + line-height: $lineheight; +} + +footer { + border-color: $footer1; +} diff --git a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/subfolder/other.scss b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/subfolder/other.scss new file mode 100644 index 00000000000..cce56d40336 --- /dev/null +++ b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/subfolder/other.scss @@ -0,0 +1,6 @@ +$footer1: #cc0000; + +code { + // ensures core variables are available + font-size: $font-down-2; +} diff --git a/spec/fixtures/plugins/scss_plugin/plugin.rb b/spec/fixtures/plugins/scss_plugin/plugin.rb new file mode 100644 index 00000000000..1a5845193b0 --- /dev/null +++ b/spec/fixtures/plugins/scss_plugin/plugin.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +# name: scss_plugin +# about: Fixture plugin for SCSS tests +# version: 1.0 + +register_asset "stylesheets/common/common.scss" diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index a6144a92d63..20e00e06940 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -154,7 +154,7 @@ HTML field.value = "@import 'missingfile';" field.save! field.ensure_baked! - expect(field.error).to include("File to import not found or unreadable: missingfile.scss.") + expect(field.error).to include("File to import not found or unreadable: missingfile") field.value = "body {color: blue};" field.save!