From f57a49c2f97c78865a4ad806339a2f847d6bc98c Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 26 Feb 2021 07:44:15 -0500 Subject: [PATCH] DEV: Use separate files for theme component stylesheets (#12214) This switches to outputting a separate file for each theme component CSS asset. We have separate CSS plugin files, separate JS files (for plugins/themes/components), it makes sense to do the same for component CSS assets. Benefits: - easier debugging - fixes a regression with theme component sourcemaps - changes to theme components are updated individually With HTTP/2, there is also no performance downside to having additional files in the initial request. --- .../app/initializers/live-development.js | 4 +- app/models/theme.rb | 5 +-- lib/stylesheet/importer.rb | 7 +--- lib/stylesheet/manager.rb | 2 +- spec/components/stylesheet/manager_spec.rb | 41 ++++++++++++++++--- spec/models/theme_spec.rb | 13 +----- 6 files changed, 41 insertions(+), 31 deletions(-) diff --git a/app/assets/javascripts/discourse/app/initializers/live-development.js b/app/assets/javascripts/discourse/app/initializers/live-development.js index 00f1f89d0ba..058865af3c4 100644 --- a/app/assets/javascripts/discourse/app/initializers/live-development.js +++ b/app/assets/javascripts/discourse/app/initializers/live-development.js @@ -1,7 +1,7 @@ -import { currentThemeIds, refreshCSS } from "discourse/lib/theme-selector"; 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 { @@ -63,13 +63,11 @@ export default { // Refresh if necessary document.location.reload(true); } else { - const themeIds = currentThemeIds(); $("link").each(function () { if (me.hasOwnProperty("theme_id") && me.new_href) { const target = $(this).data("target"); const themeId = $(this).data("theme-id"); if ( - themeIds.indexOf(me.theme_id) !== -1 && target === me.target && (!themeId || themeId === me.theme_id) ) { diff --git a/app/models/theme.rb b/app/models/theme.rb index 418b3595349..e6120778ff1 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -315,8 +315,7 @@ class Theme < ActiveRecord::Base if all_themes message = theme_ids.map { |id| refresh_message_for_targets(targets, id) }.flatten else - parent_ids = Theme.where(id: theme_ids).joins(:parent_themes).pluck(:parent_theme_id).uniq - message = refresh_message_for_targets(targets, theme_ids | parent_ids).flatten + message = refresh_message_for_targets(targets, theme_ids).flatten end MessageBus.publish('/file-change', message) @@ -372,7 +371,7 @@ class Theme < ActiveRecord::Base end def list_baked_fields(target, name) - theme_ids = Theme.transform_ids([id]) + theme_ids = Theme.transform_ids([id], extend: false) self.class.list_baked_fields(theme_ids, target, name) end diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb index b4d65498f4b..d339fe8e654 100644 --- a/lib/stylesheet/importer.rb +++ b/lib/stylesheet/importer.rb @@ -194,18 +194,13 @@ module Stylesheet fields.map do |field| value = field.value if value.present? - filename = "theme_#{field.theme.id}/#{field.target_name}-#{field.name}-#{field.theme.name.parameterize}.scss" contents << <<~COMMENT // Theme: #{field.theme.name} // Target: #{field.target_name} #{field.name} // Last Edited: #{field.updated_at} COMMENT - if field.theme_id == theme.id - contents << value - else - contents << field.compiled_css(prepended_scss) - end + contents << value end end diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index 1689a1acef5..c23a7eaa11c 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -58,7 +58,7 @@ class Stylesheet::Manager theme_ids = [theme_ids] unless Array === theme_ids theme_ids = [theme_ids.first] unless target =~ THEME_REGEX - theme_ids = Theme.transform_ids(theme_ids, extend: false) + theme_ids = Theme.transform_ids(theme_ids) current_hostname = Discourse.current_hostname diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index 22b756de94d..4ffff1744bc 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -42,7 +42,7 @@ describe Stylesheet::Manager do theme.add_relative_theme!(:child, child_theme) - old_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) + old_links = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) manager = Stylesheet::Manager.new(:desktop_theme, theme.id) manager.compile(force: true) @@ -50,27 +50,56 @@ describe Stylesheet::Manager do css = File.read(manager.stylesheet_fullpath) _source_map = File.read(manager.source_map_fullpath) - expect(css).to match(/child_common/) - expect(css).to match(/child_desktop/) expect(css).to match(/\.common/) expect(css).to match(/\.desktop/) + # child theme CSS is no longer bundled with main theme + expect(css).not_to match(/child_common/) + expect(css).not_to match(/child_desktop/) + + child_theme_manager = Stylesheet::Manager.new(:desktop_theme, child_theme.id) + child_theme_manager.compile(force: true) + + child_css = File.read(child_theme_manager.stylesheet_fullpath) + _child_source_map = File.read(child_theme_manager.source_map_fullpath) + + expect(child_css).to match(/child_common/) + expect(child_css).to match(/child_desktop/) + child_theme.set_field(target: :desktop, name: :scss, value: ".nothing{color: green;}") child_theme.save! - new_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) + new_links = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) - expect(new_link).not_to eq(old_link) + expect(new_links).not_to eq(old_links) # our theme better have a name with the theme_id as part of it - expect(new_link).to include("/stylesheets/desktop_theme_#{theme.id}_") + expect(new_links).to include("/stylesheets/desktop_theme_#{theme.id}_") + expect(new_links).to include("/stylesheets/desktop_theme_#{child_theme.id}_") manager = Stylesheet::Manager.new(:embedded_theme, theme.id) manager.compile(force: true) css = File.read(manager.stylesheet_fullpath) expect(css).to match(/\.embedded/) + expect(css).not_to match(/\.child_embedded/) + + child_theme_manager = Stylesheet::Manager.new(:embedded_theme, child_theme.id) + child_theme_manager.compile(force: true) + + css = File.read(child_theme_manager.stylesheet_fullpath) expect(css).to match(/\.child_embedded/) + + # ensure theme + component stylesheets are included + hrefs = Stylesheet::Manager.stylesheet_details(:desktop, 'all', [theme.id]) + expect(hrefs.count).to eq(2) + expect(hrefs[0][:theme_id]).to eq(theme.id) + expect(hrefs[1][:theme_id]).to eq(child_theme.id) + + hrefs = Stylesheet::Manager.stylesheet_details(:embedded_theme, 'all', [theme.id]) + expect(hrefs.count).to eq(2) + expect(hrefs[0][:theme_id]).to eq(theme.id) + expect(hrefs[1][:theme_id]).to eq(child_theme.id) end describe 'digest' do diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index 3a65d8ab265..f19cdb38191 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -795,24 +795,13 @@ HTML expect(css).to include("p{color:blue}") end - it "works for child themes and includes child theme SCSS in parent theme" do + it "works for child themes" do child_theme.set_field(target: :common, name: :scss, value: '@import "my_files/moremagic"') child_theme.save! manager = Stylesheet::Manager.new(:desktop_theme, child_theme.id) css, _map = manager.compile(force: true) expect(css).to include("body{background:green}") - - parent_css, _parent_map = compiler - 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