DEV: Refactor plugin SCSS compilation (#12359)

lib/stylesheet/common.rb

@@ -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

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]
+      elsif plugin_assets = Importer.plugin_assets[asset.to_s]
-        filename = "theme_#{options[:theme_id]}.scss"
+        filename = "#{asset.to_s}.scss"
-        file += " @import \"#{asset}\";"
+        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
+        when Stylesheet::Manager::COLOR_SCHEME_STYLESHEET
-
-        if asset.to_s == 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,

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
+    def self.plugin_assets
-      @special_imports ||= {}
+      @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?
+          plugin_assets[asset_name] = stylesheets[plugin_directory_name] if plugin_directory_name.present?
-            register_import asset_name do
-              import_files(stylesheets[plugin_directory_name])
-            end
-          end
         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

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

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
+        plugin1 = Plugin::Instance.new
-        plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb"
+        plugin1.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb"
-        plugin.register_css "body { background: $primary }"
+        plugin1.register_css "body { background: $primary }"
-        Discourse.plugins << plugin
+
-        plugin.activate!
+        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
 

spec/fixtures/plugins/scss_plugin/assets/stylesheets/colors.scss

@@ -0,0 +1,2 @@
+$color1: red;
+$color2: green;

spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/_variables.scss

@@ -0,0 +1 @@
+$lineheight: 1.2em;

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;
+}

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;
+}

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"

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!