From aa1442fdc392444fc0f1c0d1f1468fbdb5b55898 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 26 Feb 2021 12:30:23 -0500 Subject: [PATCH] DEV: Use separate files for theme component stylesheets (take 2) (#12225) 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 | 4 +- spec/components/stylesheet/manager_spec.rb | 109 ++++++++++++------ spec/models/theme_spec.rb | 13 +-- 6 files changed, 83 insertions(+), 59 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..ab3465b0906 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -58,7 +58,9 @@ 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) + include_components = !!(target =~ THEME_REGEX) + + theme_ids = Theme.transform_ids(theme_ids, extend: include_components) current_hostname = Discourse.current_hostname diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index 22b756de94d..5ce87c428fe 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -22,55 +22,96 @@ describe Stylesheet::Manager do expect(link).not_to eq("") end - it 'can correctly compile theme css' do - theme = Fabricate(:theme) + context "themes with components" do + let(:child_theme) { Fabricate(:theme, component: true).tap { |c| + c.set_field(target: :common, name: "scss", value: ".child_common{.scss{color: red;}}") + c.set_field(target: :desktop, name: "scss", value: ".child_desktop{.scss{color: red;}}") + c.set_field(target: :mobile, name: "scss", value: ".child_mobile{.scss{color: red;}}") + c.set_field(target: :common, name: "embedded_scss", value: ".child_embedded{.scss{color: red;}}") + c.save! + }} - theme.set_field(target: :common, name: "scss", value: ".common{.scss{color: red;}}") - theme.set_field(target: :desktop, name: "scss", value: ".desktop{.scss{color: red;}}") - theme.set_field(target: :mobile, name: "scss", value: ".mobile{.scss{color: red;}}") - theme.set_field(target: :common, name: "embedded_scss", value: ".embedded{.scss{color: red;}}") + let(:theme) { Fabricate(:theme).tap { |t| + t.set_field(target: :common, name: "scss", value: ".common{.scss{color: red;}}") + t.set_field(target: :desktop, name: "scss", value: ".desktop{.scss{color: red;}}") + t.set_field(target: :mobile, name: "scss", value: ".mobile{.scss{color: red;}}") + t.set_field(target: :common, name: "embedded_scss", value: ".embedded{.scss{color: red;}}") + t.save! - theme.save! + t.add_relative_theme!(:child, child_theme) + }} - child_theme = Fabricate(:theme, component: true) + it 'can correctly compile theme css' do + old_links = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) - child_theme.set_field(target: :common, name: "scss", value: ".child_common{.scss{color: red;}}") - child_theme.set_field(target: :desktop, name: "scss", value: ".child_desktop{.scss{color: red;}}") - child_theme.set_field(target: :mobile, name: "scss", value: ".child_mobile{.scss{color: red;}}") - child_theme.set_field(target: :common, name: "embedded_scss", value: ".child_embedded{.scss{color: red;}}") - child_theme.save! + manager = Stylesheet::Manager.new(:desktop_theme, theme.id) + manager.compile(force: true) - theme.add_relative_theme!(:child, child_theme) + css = File.read(manager.stylesheet_fullpath) + _source_map = File.read(manager.source_map_fullpath) - old_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) + expect(css).to match(/\.common/) + expect(css).to match(/\.desktop/) - manager = Stylesheet::Manager.new(:desktop_theme, theme.id) - manager.compile(force: true) + # child theme CSS is no longer bundled with main theme + expect(css).not_to match(/child_common/) + expect(css).not_to match(/child_desktop/) - css = File.read(manager.stylesheet_fullpath) - _source_map = File.read(manager.source_map_fullpath) + child_theme_manager = Stylesheet::Manager.new(:desktop_theme, child_theme.id) + child_theme_manager.compile(force: true) - expect(css).to match(/child_common/) - expect(css).to match(/child_desktop/) - expect(css).to match(/\.common/) - expect(css).to match(/\.desktop/) + child_css = File.read(child_theme_manager.stylesheet_fullpath) + _child_source_map = File.read(child_theme_manager.source_map_fullpath) - child_theme.set_field(target: :desktop, name: :scss, value: ".nothing{color: green;}") - child_theme.save! + expect(child_css).to match(/child_common/) + expect(child_css).to match(/child_desktop/) - new_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) + child_theme.set_field(target: :desktop, name: :scss, value: ".nothing{color: green;}") + child_theme.save! - expect(new_link).not_to eq(old_link) + new_links = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) - # 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).not_to eq(old_links) - manager = Stylesheet::Manager.new(:embedded_theme, theme.id) - manager.compile(force: true) + # our theme better have a name with the theme_id as part of it + expect(new_links).to include("/stylesheets/desktop_theme_#{theme.id}_") + expect(new_links).to include("/stylesheets/desktop_theme_#{child_theme.id}_") + end - css = File.read(manager.stylesheet_fullpath) - expect(css).to match(/\.embedded/) - expect(css).to match(/\.child_embedded/) + it 'can correctly compile embedded theme css' do + 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/) + end + + it 'includes both parent and child theme assets' do + hrefs = Stylesheet::Manager.stylesheet_details(:desktop_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) + + 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 + + it 'does not output multiple assets for non-themes' do + hrefs = Stylesheet::Manager.stylesheet_details(:admin, 'all', [theme.id]) + expect(hrefs.count).to eq(1) + + hrefs = Stylesheet::Manager.stylesheet_details(:mobile, 'all', [theme.id]) + expect(hrefs.count).to eq(1) + end 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