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.
This commit is contained in:
Penar Musaraj 2021-02-26 12:30:23 -05:00 committed by GitHub
parent 0581c033d7
commit aa1442fdc3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 83 additions and 59 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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