From 12ffba771ccde1090821f06c254fa91b9f751a44 Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Thu, 4 Feb 2021 08:51:18 -0500
Subject: [PATCH] FIX: Improve SCSS handling in components (#11963)

- ignores errors when including component SCSS in parent theme
- adds support for SCSS `@import`s in components' `color_definitions.scss` files
---
 app/models/theme_field.rb                   | 12 ++++++++++++
 lib/stylesheet/importer.rb                  | 20 ++++++++++----------
 spec/components/stylesheet/importer_spec.rb |  7 ++++---
 spec/models/theme_spec.rb                   |  7 +++++++
 4 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb
index 38701933ea8..540387a5e36 100644
--- a/app/models/theme_field.rb
+++ b/app/models/theme_field.rb
@@ -354,6 +354,18 @@ class ThemeField < ActiveRecord::Base
     )
   end
 
+  def compiled_css
+    css, _source_map = begin
+      compile_scss
+    rescue SassC::SyntaxError => e
+      # We don't want to raise a blocking error here
+      # admin theme editor or discourse_theme CLI will show it nonetheless
+      Rails.logger.error "SCSS compilation error: #{e.message}"
+      ["", nil]
+    end
+    css
+  end
+
   def ensure_scss_compiles!
     result = ["failed"]
     begin
diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb
index 20989916522..6f241f77d55 100644
--- a/lib/stylesheet/importer.rb
+++ b/lib/stylesheet/importer.rb
@@ -141,9 +141,15 @@ module Stylesheet
       if resolved_ids
         theme = Theme.find_by_id(theme_id)
         contents << theme&.scss_variables.to_s
-        Theme.list_baked_fields(resolved_ids, :common, :color_definitions).each do |row|
-          contents << "// Color definitions from #{theme.name}\n\n"
-          contents << row.value
+        Theme.list_baked_fields(resolved_ids, :common, :color_definitions).each do |field|
+          contents << "// Color definitions from #{field.theme.name}\n\n"
+
+          if field.theme_id == theme.id
+            contents << field.value
+          else
+            contents << field.compiled_css
+          end
+          contents << "\n\n"
         end
       end
       contents
@@ -198,13 +204,7 @@ module Stylesheet
           if field.theme_id == theme.id
             contents << value
           else
-            css, source_map = begin
-              field.compile_scss
-            rescue SassC::SyntaxError => e
-              raise Discourse::ScssError, e.message
-            end
-
-            contents << css
+            contents << field.compiled_css
           end
         end
 
diff --git a/spec/components/stylesheet/importer_spec.rb b/spec/components/stylesheet/importer_spec.rb
index 36ee1181358..acb66072c27 100644
--- a/spec/components/stylesheet/importer_spec.rb
+++ b/spec/components/stylesheet/importer_spec.rb
@@ -64,8 +64,8 @@ describe Stylesheet::Importer do
   end
 
   context "#import_color_definitions" do
-    let(:scss) { ":root { --custom-color: green}" }
-    let(:scss_child) { ":root { --custom-color: red}" }
+    let(:scss) { ":root{--custom-color: green}" }
+    let(:scss_child) { ":root{--custom-color: red}" }
 
     let(:theme) do
       Fabricate(:theme).tap do |t|
@@ -74,7 +74,7 @@ describe Stylesheet::Importer do
       end
     end
 
-    let(:child) { Fabricate(:theme, component: true).tap { |t|
+    let(:child) { Fabricate(:theme, component: true, name: "Child Theme").tap { |t|
       t.set_field(target: :common, name: "color_definitions", value: scss_child)
       t.save!
     }}
@@ -90,6 +90,7 @@ describe Stylesheet::Importer do
 
       styles = Stylesheet::Importer.import_color_definitions(theme.id)
       expect(styles).to include(scss_child)
+      expect(styles).to include("Color definitions from Child Theme")
     end
 
     it "should include default theme color definitions" do
diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb
index eca11336a00..3a65d8ab265 100644
--- a/spec/models/theme_spec.rb
+++ b/spec/models/theme_spec.rb
@@ -807,6 +807,13 @@ HTML
       expect(parent_css).to include("body{background:green}")
     end
 
+    it "does not fail if child theme has SCSS errors" do
+      child_theme.set_field(target: :common, name: :scss, value: 'p {color: $missing_var;}')
+      child_theme.save!
+
+      parent_css, _parent_map = compiler
+      expect(parent_css).to include("sourceMappingURL")
+    end
   end
 
   describe "scss_variables" do