DEV: Revert stylesheet refactors (#13584)

* Revert "FIX: Clear appropriate cache when updating font settings (#13582)"

This reverts commit de6cc7a924.

* Revert "DEV: Improve output of `Stylesheet::Mananger.precompile_theme_css`."

This reverts commit 95038856c9.

* Revert "FIX: Child themes being precompiled multiple times."

This reverts commit 6986b36985.

* Revert "Update spec/components/stylesheet/manager_spec.rb"

This reverts commit ddaa7cc7ea.

* Revert "Refactor scss live refreshing"

This reverts commit a838293aaf.

* Revert "Precompile core stylesheets independently of themes"

This reverts commit 99d259d39b.

* Revert "DEV: Add simple digest for core stylesheets"

This reverts commit d82c58e6cc.

app/assets/javascripts/discourse/app/initializers/live-development.js

@@ -1,6 +1,7 @@
 import DiscourseURL from "discourse/lib/url";
 import Handlebars from "handlebars";
 import { isDevelopment } from "discourse-common/config/environment";
+import { refreshCSS } from "discourse/lib/theme-selector";
 
 //  Use the message bus for live reloading of components for faster development.
 export default {
@@ -50,7 +51,7 @@ export default {
     // Observe file changes
     messageBus.subscribe(
       "/file-change",
-      (data) => {
+      function (data) {
         if (Handlebars.compile && !Ember.TEMPLATES.empty) {
           // hbs notifications only happen in dev
           Ember.TEMPLATES.empty = Handlebars.compile("<div></div>");
@@ -59,12 +60,20 @@ export default {
           if (me === "refresh") {
             // Refresh if necessary
             document.location.reload(true);
-          } else if (me.new_href && me.target) {
+          } else {
-            const link_target = me.theme_id
+            $("link").each(function () {
-              ? `[data-target="${me.target}"][data-theme-id="${me.theme_id}"]`
+              if (me.hasOwnProperty("theme_id") && me.new_href) {
-              : `[data-target="${me.target}"]`;
+                const target = $(this).data("target");
-            document.querySelectorAll(`link${link_target}`).forEach((link) => {
+                const themeId = $(this).data("theme-id");
-              this.refreshCSS(link, me.new_href);
+                if (
+                  target === me.target &&
+                  (!themeId || themeId === me.theme_id)
+                ) {
+                  refreshCSS(this, null, me.new_href);
+                }
+              } else if (this.href.match(me.name) && (me.hash || me.new_href)) {
+                refreshCSS(this, me.hash, me.new_href);
+              }
             });
           }
         });
@@ -72,23 +81,4 @@ export default {
       session.mbLastFileChangeId
     );
   },
-
-  refreshCSS(node, newHref) {
-    if (node.dataset.reloading) {
-      clearTimeout(node.dataset.timeout);
-    }
-
-    node.dataset.reloading = true;
-
-    let reloaded = node.cloneNode(true);
-    reloaded.href = newHref;
-    node.insertAdjacentElement("afterend", reloaded);
-
-    let timeout = setTimeout(() => {
-      node.parentNode.removeChild(node);
-      reloaded.dataset.reloading = false;
-    }, 2000);
-
-    node.dataset.timeout = timeout;
-  },
 };

app/assets/javascripts/discourse/app/lib/theme-selector.js

@@ -44,6 +44,41 @@ export function setLocalTheme(ids, themeSeq) {
   }
 }
 
+export function refreshCSS(node, hash, newHref) {
+  let $orig = $(node);
+
+  if ($orig.data("reloading")) {
+    clearTimeout($orig.data("timeout"));
+    $orig.data("copy").remove();
+  }
+
+  if (!$orig.data("orig")) {
+    $orig.data("orig", node.href);
+  }
+
+  $orig.data("reloading", true);
+
+  const orig = $(node).data("orig");
+
+  let reloaded = $orig.clone(true);
+  if (hash) {
+    reloaded[0].href =
+      orig + (orig.indexOf("?") >= 0 ? "&hash=" : "?hash=") + hash;
+  } else {
+    reloaded[0].href = newHref;
+  }
+
+  $orig.after(reloaded);
+
+  let timeout = setTimeout(() => {
+    $orig.remove();
+    reloaded.data("reloading", false);
+  }, 2000);
+
+  $orig.data("timeout", timeout);
+  $orig.data("copy", reloaded);
+}
+
 export function listThemes(site) {
   let themes = site.get("user_themes");
 

config/initializers/014-track-setting-changes.rb

@@ -27,7 +27,7 @@ DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value|
     end
   end
 
-  Stylesheet::Manager.clear_color_scheme_cache! if [:base_font, :heading_font].include?(name)
+  Stylesheet::Manager.clear_core_cache!(["desktop", "mobile"]) if [:base_font, :heading_font].include?(name)
 
   Report.clear_cache(:storage_stats) if [:backup_location, :s3_backup_bucket].include?(name)
 

lib/stylesheet/compiler.rb

@@ -29,6 +29,9 @@ module Stylesheet
         file += File.read path
 
         case asset.to_s
+        when "desktop", "mobile"
+          file += importer.category_backgrounds
+          file += importer.font
         when "embed", "publish"
           file += importer.font
         when "wizard"
@@ -36,8 +39,6 @@ module Stylesheet
         when Stylesheet::Manager::COLOR_SCHEME_STYLESHEET
           file += importer.import_color_definitions
           file += importer.import_wcag_overrides
-          file += importer.category_backgrounds
-          file += importer.font
         end
       end
 

lib/stylesheet/manager.rb

@@ -42,58 +42,52 @@ class Stylesheet::Manager
   end
 
   def self.precompile_css
-    targets = [:desktop, :mobile, :desktop_rtl, :mobile_rtl, :admin, :wizard]
+    themes = Theme.where('user_selectable OR id = ?', SiteSetting.default_theme_id).pluck(:id, :name, :color_scheme_id)
-    targets += Discourse.find_plugin_css_assets(include_disabled: true, mobile_view: true, desktop_view: true)
+    themes << nil
-
-    targets.each do |target|
-      $stderr.puts "precompile target: #{target}"
-
-      Stylesheet::Manager::Builder.new(target: target, manager: nil).compile(force: true)
-    end
-  end
-
-  def self.precompile_theme_css
-    themes = Theme.where('user_selectable OR id = ?', SiteSetting.default_theme_id).pluck(:id, :color_scheme_id)
 
     color_schemes = ColorScheme.where(user_selectable: true).to_a
     color_schemes << ColorScheme.find_by(id: SiteSetting.default_dark_mode_color_scheme_id)
-    color_schemes << ColorScheme.base
     color_schemes = color_schemes.compact.uniq
 
-    targets = [:desktop_theme, :mobile_theme]
+    targets = [:desktop, :mobile, :desktop_rtl, :mobile_rtl, :desktop_theme, :mobile_theme, :admin, :wizard]
-    compiled = Set.new
+    targets += Discourse.find_plugin_css_assets(include_disabled: true, mobile_view: true, desktop_view: true)
 
-    themes.each do |theme_id, color_scheme_id|
+    themes.each do |id, name, color_scheme_id|
+      theme_id = id || SiteSetting.default_theme_id
       manager = self.new(theme_id: theme_id)
 
       targets.each do |target|
+        if target =~ THEME_REGEX
           next if theme_id == -1
 
           scss_checker = ScssChecker.new(target, manager.theme_ids)
 
           manager.load_themes(manager.theme_ids).each do |theme|
-          next if compiled.include?("#{target}_#{theme.id}")
-
             builder = Stylesheet::Manager::Builder.new(
               target: target, theme: theme, manager: manager
             )
 
+            STDERR.puts "precompile target: #{target} #{builder.theme.name}"
             next if theme.component && !scss_checker.has_scss(theme.id)
-          $stderr.puts "precompile target: #{target} #{theme.name}"
             builder.compile(force: true)
-          compiled << "#{target}_#{theme.id}"
+          end
+        else
+          STDERR.puts "precompile target: #{target} #{name}"
+
+          Stylesheet::Manager::Builder.new(
+            target: target, theme: manager.get_theme(theme_id), manager: manager
+          ).compile(force: true)
         end
       end
 
-      theme_color_scheme = ColorScheme.find_by_id(color_scheme_id)
+      theme_color_scheme = ColorScheme.find_by_id(color_scheme_id) || ColorScheme.base
-      theme = manager.get_theme(theme_id)
 
-      [theme_color_scheme, *color_schemes].compact.uniq.each do |scheme|
+      [theme_color_scheme, *color_schemes].uniq.each do |scheme|
-        $stderr.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} #{theme.name} (#{scheme.name})"
+        STDERR.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} #{name} (#{scheme.name})"
 
         Stylesheet::Manager::Builder.new(
           target: COLOR_SCHEME_STYLESHEET,
-          theme: theme,
+          theme: manager.get_theme(theme_id),
           color_scheme: scheme,
           manager: manager
         ).compile(force: true)
@@ -199,19 +193,15 @@ class Stylesheet::Manager
   def stylesheet_details(target = :desktop, media = 'all')
     target = target.to_sym
     current_hostname = Discourse.current_hostname
-    is_theme_target = !!(target.to_s =~ THEME_REGEX)
-
-    array_cache_key = is_theme_target ?
-      "array_themes_#{@theme_ids.join(",")}_#{target}_#{current_hostname}" :
-      "array_#{target}_#{current_hostname}"
 
+    array_cache_key = "array_themes_#{@theme_ids.join(",")}_#{target}_#{current_hostname}"
     stylesheets = cache[array_cache_key]
     return stylesheets if stylesheets.present?
 
     @@lock.synchronize do
       stylesheets = []
       stale_theme_ids = []
-      theme_ids = is_theme_target ? @theme_ids : [nil]
+      theme_ids = target.to_s =~ THEME_REGEX ? @theme_ids : [@theme_id]
 
       theme_ids.each do |theme_id|
         cache_key = "path_#{target}_#{theme_id}_#{current_hostname}"
@@ -229,8 +219,7 @@ class Stylesheet::Manager
 
       scss_checker = ScssChecker.new(target, stale_theme_ids)
 
-      if is_theme_target
+      themes = @theme_id.blank? ? [nil] : load_themes(stale_theme_ids)
-        themes = load_themes(stale_theme_ids)
 
       themes.each do |theme|
         theme_id = theme&.id
@@ -251,16 +240,6 @@ class Stylesheet::Manager
         data[:new_href] = href
         stylesheets << data
       end
-      else
-        builder = Builder.new(target: target, manager: self)
-        builder.compile unless File.exists?(builder.stylesheet_fullpath)
-        href = builder.stylesheet_path(current_hostname)
-
-        cache.defer_set("path_#{target}__#{current_hostname}", href)
-
-        data = { target: target, new_href: href }
-        stylesheets << data
-      end
 
       cache.defer_set(array_cache_key, stylesheets.freeze)
       stylesheets

lib/stylesheet/manager/builder.rb

@@ -137,10 +137,6 @@ class Stylesheet::Manager::Builder
     !!(@target.to_s =~ Stylesheet::Manager::THEME_REGEX)
   end
 
-  def is_color_scheme?
-    !!(@target.to_s == Stylesheet::Manager::COLOR_SCHEME_STYLESHEET)
-  end
-
   def scheme_slug
     Slug.for(ActiveSupport::Inflector.transliterate(@color_scheme.name), 'scheme')
   end
@@ -150,10 +146,8 @@ class Stylesheet::Manager::Builder
     @digest ||= begin
       if is_theme?
         theme_digest
-      elsif is_color_scheme?
-        color_scheme_digest
       else
-        default_digest
+        color_scheme_digest
       end
     end
   end
@@ -177,7 +171,7 @@ class Stylesheet::Manager::Builder
   end
 
   def theme_digest
-    Digest::SHA1.hexdigest(scss_digest.to_s + color_scheme_digest.to_s + settings_digest + uploads_digest)
+    Digest::SHA1.hexdigest(scss_digest.to_s + color_scheme_digest.to_s + settings_digest + plugins_digest + uploads_digest)
   end
 
   # this protects us from situations where new versions of a plugin removed a file
@@ -224,10 +218,6 @@ class Stylesheet::Manager::Builder
     Digest::SHA1.hexdigest(sha1s.sort!.join("\n"))
   end
 
-  def default_digest
-    Digest::SHA1.hexdigest "default-#{Stylesheet::Manager.last_file_updated}-#{plugins_digest}"
-  end
-
   def color_scheme_digest
     cs = @color_scheme || theme&.color_scheme
 

lib/stylesheet/watcher.rb

@@ -96,7 +96,11 @@ module Stylesheet
       targets = target ? [target] : ["desktop", "mobile", "admin"]
       Stylesheet::Manager.clear_core_cache!(targets)
       message = targets.map! do |name|
-        Stylesheet::Manager.new.stylesheet_data(name.to_sym)
+        msgs = []
+        active_themes.each do |theme_id|
+          msgs << Stylesheet::Manager.new(theme_id: theme_id).stylesheet_data(name.to_sym)
+        end
+        msgs
       end.flatten!
       MessageBus.publish '/file-change', message
     end
@@ -110,7 +114,11 @@ module Stylesheet
         targets.push(plugin_name)
       end
       message = targets.map! do |name|
-        Stylesheet::Manager.new.stylesheet_data(name.to_sym)
+        msgs = []
+        active_themes.each do |theme_id|
+          msgs << Stylesheet::Manager.new(theme_id: theme_id).stylesheet_data(name.to_sym)
+        end
+        msgs
       end.flatten!
       MessageBus.publish '/file-change', message
     end
@@ -135,5 +143,9 @@ module Stylesheet
       end
     end
 
+    def active_themes
+      @active_themes ||= Theme.user_selectable.pluck(:id)
+    end
+
   end
 end

lib/tasks/assets.rake

@@ -51,8 +51,7 @@ task 'assets:precompile:css' => 'environment' do
       if ActiveRecord::Base.connection.table_exists?(Theme.table_name)
         STDERR.puts "Compiling css for #{db} #{Time.zone.now}"
         begin
-          Stylesheet::Manager.precompile_css if db == "default"
+          Stylesheet::Manager.precompile_css
-          Stylesheet::Manager.precompile_theme_css
         rescue PG::UndefinedColumn, ActiveModel::MissingAttributeError, NoMethodError => e
           STDERR.puts "#{e.class} #{e.message}: #{e.backtrace.join("\n")}"
           STDERR.puts "Skipping precompilation of CSS cause schema is old, you are precompiling prior to running migrations."

spec/components/stylesheet/importer_spec.rb

@@ -11,16 +11,19 @@ describe Stylesheet::Importer do
 
   context "#category_backgrounds" do
     it "applies CDN to background category images" do
-      expect(compile_css("color_definitions")).to_not include("body.category-")
+      expect(compile_css("mobile")).to_not include("body.category-")
+      expect(compile_css("desktop")).to_not include("body.category-")
 
       background = Fabricate(:upload)
       parent_category = Fabricate(:category)
       category = Fabricate(:category, parent_category_id: parent_category.id, uploaded_background: background)
 
-      expect(compile_css("color_definitions")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(#{background.url})}")
+      expect(compile_css("mobile")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(#{background.url})}")
+      expect(compile_css("desktop")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(#{background.url})}")
 
       GlobalSetting.stubs(:cdn_url).returns("//awesome.cdn")
-      expect(compile_css("color_definitions")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(//awesome.cdn#{background.url})}")
+      expect(compile_css("mobile")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(//awesome.cdn#{background.url})}")
+      expect(compile_css("desktop")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(//awesome.cdn#{background.url})}")
     end
 
     it "applies S3 CDN to background category images" do
@@ -33,7 +36,8 @@ describe Stylesheet::Importer do
       background = Fabricate(:upload_s3)
       category = Fabricate(:category, uploaded_background: background)
 
-      expect(compile_css("color_definitions")).to include("body.category-#{category.slug}{background-image:url(https://s3.cdn/original")
+      expect(compile_css("mobile")).to include("body.category-#{category.slug}{background-image:url(https://s3.cdn/original")
+      expect(compile_css("desktop")).to include("body.category-#{category.slug}{background-image:url(https://s3.cdn/original")
     end
 
   end
@@ -41,7 +45,8 @@ describe Stylesheet::Importer do
   context "#font" do
     it "includes font variable" do
       default_font = ":root{--font-family: Arial, sans-serif}"
-      expect(compile_css("color_definitions")).to include(default_font)
+      expect(compile_css("desktop")).to include(default_font)
+      expect(compile_css("mobile")).to include(default_font)
       expect(compile_css("embed")).to include(default_font)
       expect(compile_css("publish")).to include(default_font)
     end
@@ -53,14 +58,14 @@ describe Stylesheet::Importer do
       SiteSetting.base_font = base_font[:key]
       SiteSetting.heading_font = heading_font[:key]
 
-      expect(compile_css("color_definitions"))
+      expect(compile_css("desktop"))
         .to include(":root{--font-family: #{base_font[:stack]}}")
         .and include(":root{--heading-font-family: #{heading_font[:stack]}}")
 
       set_cdn_url("http://cdn.localhost")
 
       # uses CDN and includes cache-breaking param
-      expect(compile_css("color_definitions"))
+      expect(compile_css("mobile"))
         .to include("http://cdn.localhost/fonts/#{base_font[:variants][0][:filename]}?v=#{DiscourseFonts::VERSION}")
         .and include("http://cdn.localhost/fonts/#{heading_font[:variants][0][:filename]}?v=#{DiscourseFonts::VERSION}")
     end

spec/components/stylesheet/manager_spec.rb

@@ -39,7 +39,7 @@ describe Stylesheet::Manager do
     }}
 
     it "generates the right links for non-theme targets" do
-      manager = manager(nil)
+      manager = manager(theme.id)
 
       hrefs = manager.stylesheet_details(:desktop, 'all')
 
@@ -188,12 +188,22 @@ describe Stylesheet::Manager do
       DiscoursePluginRegistry.reset!
     end
 
-    it 'can correctly account for plugins in default digest' do
+    it 'can correctly account for plugins in digest' do
-      builder = Stylesheet::Manager::Builder.new(target: :desktop, manager: manager)
+      theme = Fabricate(:theme)
+      manager = manager(theme.id)
+
+      builder = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      )
+
       digest1 = builder.digest
 
       DiscoursePluginRegistry.stylesheets["fake"] = Set.new(["fake_file"])
-      builder = Stylesheet::Manager::Builder.new(target: :desktop, manager: manager)
+
+      builder = Stylesheet::Manager::Builder.new(
+        target: :desktop_theme, theme: theme, manager: manager
+      )
+
       digest2 = builder.digest
 
       expect(digest1).not_to eq(digest2)
@@ -272,21 +282,6 @@ describe Stylesheet::Manager do
 
       expect(digest1).not_to eq(digest2)
     end
-
-    it 'returns different digest based on target' do
-      theme = Fabricate(:theme)
-      builder = Stylesheet::Manager::Builder.new(target: :desktop_theme, theme: theme, manager: manager)
-      expect(builder.digest).to eq(builder.theme_digest)
-
-      builder = Stylesheet::Manager::Builder.new(target: :color_definitions, manager: manager)
-      expect(builder.digest).to eq(builder.color_scheme_digest)
-
-      builder = Stylesheet::Manager::Builder.new(target: :admin, manager: manager)
-      expect(builder.digest).to eq(builder.default_digest)
-
-      builder = Stylesheet::Manager::Builder.new(target: :desktop, manager: manager)
-      expect(builder.digest).to eq(builder.default_digest)
-    end
   end
 
   describe 'color_scheme_digest' do
@@ -502,15 +497,6 @@ describe Stylesheet::Manager do
       expect(stylesheet2).to include("--primary: #c00;")
     end
 
-    it "includes updated font definitions" do
-      details1 = manager.color_scheme_stylesheet_details(nil, "all")
-
-      SiteSetting.base_font = DiscourseFonts.fonts[2][:key]
-
-      details2 = manager.color_scheme_stylesheet_details(nil, "all")
-      expect(details1[:new_href]).not_to eq(details2[:new_href])
-    end
-
     context "theme colors" do
       let(:theme) { Fabricate(:theme).tap { |t|
         t.set_field(target: :common, name: "color_definitions", value: ':root {--special: rebeccapurple;}')
@@ -616,7 +602,8 @@ describe Stylesheet::Manager do
     end
   end
 
-  describe ".precompile css" do
+  # this test takes too long, we don't run it by default
+  describe ".precompile_css", if: ENV["RUN_LONG_TESTS"] == "1" do
     before do
       class << STDERR
         alias_method :orig_write, :write
@@ -663,35 +650,21 @@ describe Stylesheet::Manager do
         t.save!
 
         user_theme.add_relative_theme!(:child, t)
-        default_theme.add_relative_theme!(:child, t)
       end
 
       default_theme.set_default!
 
       StylesheetCache.destroy_all
 
-      # only core
       Stylesheet::Manager.precompile_css
       results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(core_targets.size)
 
-      StylesheetCache.destroy_all
+      expect(results.size).to eq(24) # (2 themes x 8 targets) + (1 child Theme x 2 targets) + 6 color schemes (2 custom theme schemes, 4 base schemes)
 
-      # only themes
+      core_targets.each do |tar|
-      output = capture_output(:stderr) do
+        expect(results.count { |target| target =~ /^#{tar}_(#{scheme1.id}|#{scheme2.id})$/ }).to eq(2)
-        Stylesheet::Manager.precompile_theme_css
       end
 
-      # Ensure we force compile each theme only once
-      expect(output.scan(/#{child_theme_with_css.name}/).length).to eq(2)
-      results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(12) # (2 themes * 2 targets) + (one child theme * 2 targets) + 6 color schemes (2 custom, 4 base schemes)
-
-      # themes + core
-      Stylesheet::Manager.precompile_css
-      results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(18) # core targets + 6 theme + 6 color schemes
-
       theme_targets.each do |tar|
         expect(results.count { |target| target =~ /^#{tar}_(#{user_theme.id}|#{default_theme.id})$/ }).to eq(2)
       end
@@ -699,11 +672,18 @@ describe Stylesheet::Manager do
       Theme.clear_default!
       StylesheetCache.destroy_all
 
-      # themes + core with no theme set as default
       Stylesheet::Manager.precompile_css
-      Stylesheet::Manager.precompile_theme_css
       results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(18) # core targets + 6 theme + 6 color schemes
+
+      expect(results.size).to eq(30) # (2 themes x 8 targets) + (1 child Theme x 2 targets) + (1 no/default/core theme x 6 core targets) + 6 color schemes (2 custom theme schemes, 4 base schemes)
+
+      core_targets.each do |tar|
+        expect(results.count { |target| target =~ /^(#{tar}_(#{scheme1.id}|#{scheme2.id})|#{tar})$/ }).to eq(3)
+      end
+
+      theme_targets.each do |tar|
+        expect(results.count { |target| target =~ /^#{tar}_(#{user_theme.id}|#{default_theme.id})$/ }).to eq(2)
+      end
 
       expect(results).to include(color_scheme_targets[0])
       expect(results).to include(color_scheme_targets[1])