DEV: Compile core and plugin stylesheets independently of themes (#13638)

Take 2 of https://github.com/discourse/discourse/pull/13466. 

Fixes a few issues with the original PR: 

- color definition stylesheet target now includes the theme id, to avoid themes set to use the default color scheme loading the same stylesheet 
- changes the internal cache key for color definition stylesheet to reset the pre-existing cache
This commit is contained in:
Penar Musaraj 2021-07-06 13:11:10 -04:00 committed by GitHub
parent da03a3f5d6
commit 95b5794331
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 167 additions and 169 deletions

View File

@ -1,7 +1,6 @@
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 {
@ -51,7 +50,7 @@ export default {
// Observe file changes
messageBus.subscribe(
"/file-change",
function (data) {
(data) => {
if (Handlebars.compile && !Ember.TEMPLATES.empty) {
// hbs notifications only happen in dev
Ember.TEMPLATES.empty = Handlebars.compile("<div></div>");
@ -60,20 +59,12 @@ export default {
if (me === "refresh") {
// Refresh if necessary
document.location.reload(true);
} else {
$("link").each(function () {
if (me.hasOwnProperty("theme_id") && me.new_href) {
const target = $(this).data("target");
const themeId = $(this).data("theme-id");
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);
}
} else if (me.new_href && me.target) {
const link_target = me.theme_id
? `[data-target="${me.target}"][data-theme-id="${me.theme_id}"]`
: `[data-target="${me.target}"]`;
document.querySelectorAll(`link${link_target}`).forEach((link) => {
this.refreshCSS(link, me.new_href);
});
}
});
@ -81,4 +72,23 @@ 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;
},
};

View File

@ -44,41 +44,6 @@ 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");

View File

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

View File

@ -29,9 +29,6 @@ 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"
@ -39,6 +36,8 @@ 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

View File

@ -38,58 +38,58 @@ class Stylesheet::Manager
def self.color_scheme_cache_key(color_scheme, theme_id = nil)
color_scheme_name = Slug.for(color_scheme.name) + color_scheme&.id.to_s
theme_string = theme_id ? "_theme#{theme_id}" : ""
"#{COLOR_SCHEME_STYLESHEET}_#{color_scheme_name}#{theme_string}_#{Discourse.current_hostname}"
"#{COLOR_SCHEME_STYLESHEET}_#{color_scheme_name}_#{theme_string}_#{Discourse.current_hostname}"
end
def self.precompile_css
targets = [:desktop, :mobile, :desktop_rtl, :mobile_rtl, :admin, :wizard]
targets += Discourse.find_plugin_css_assets(include_disabled: true, mobile_view: true, desktop_view: true)
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)
themes << nil
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, :mobile, :desktop_rtl, :mobile_rtl, :desktop_theme, :mobile_theme, :admin, :wizard]
targets += Discourse.find_plugin_css_assets(include_disabled: true, mobile_view: true, desktop_view: true)
targets = [:desktop_theme, :mobile_theme]
compiled = Set.new
themes.each do |id, color_scheme_id|
theme_id = id || SiteSetting.default_theme_id
themes.each do |theme_id, color_scheme_id|
manager = self.new(theme_id: theme_id)
targets.each do |target|
if target =~ THEME_REGEX
next if theme_id == -1
next if theme_id == -1
scss_checker = ScssChecker.new(target, manager.theme_ids)
scss_checker = ScssChecker.new(target, manager.theme_ids)
manager.load_themes(manager.theme_ids).each do |theme|
next if compiled.include?("#{target}_#{theme.id}")
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
)
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
theme = manager.get_theme(theme_id)
$stderr.puts "precompile target: #{target} #{theme&.name}"
Stylesheet::Manager::Builder.new(
builder = Stylesheet::Manager::Builder.new(
target: target, theme: theme, manager: manager
).compile(force: true)
)
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
end
theme_color_scheme = ColorScheme.find_by_id(color_scheme_id) || ColorScheme.base
theme_color_scheme = ColorScheme.find_by_id(color_scheme_id)
theme = manager.get_theme(theme_id)
[theme_color_scheme, *color_schemes].uniq.each do |scheme|
$stderr.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} (#{scheme.name}) #{theme&.name}"
[theme_color_scheme, *color_schemes].compact.uniq.each do |scheme|
$stderr.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} #{theme.name} (#{scheme.name})"
Stylesheet::Manager::Builder.new(
target: COLOR_SCHEME_STYLESHEET,
@ -200,15 +200,19 @@ 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 = target.to_s =~ THEME_REGEX ? @theme_ids : [@theme_id]
theme_ids = is_theme_target ? @theme_ids : [nil]
theme_ids.each do |theme_id|
cache_key = "path_#{target}_#{theme_id}_#{current_hostname}"
@ -226,25 +230,36 @@ class Stylesheet::Manager
scss_checker = ScssChecker.new(target, stale_theme_ids)
themes = @theme_id.blank? ? [nil] : load_themes(stale_theme_ids)
if is_theme_target
themes = load_themes(stale_theme_ids)
themes.each do |theme|
theme_id = theme&.id
data = { target: target, theme_id: theme_id }
builder = Builder.new(target: target, theme: theme, manager: self)
is_theme = builder.is_theme?
has_theme = builder.theme.present?
themes.each do |theme|
theme_id = theme&.id
data = { target: target, theme_id: theme_id }
builder = Builder.new(target: target, theme: theme, manager: self)
is_theme = builder.is_theme?
has_theme = builder.theme.present?
if is_theme && !has_theme
next
else
next if is_theme && builder.theme&.component && !scss_checker.has_scss(theme_id)
builder.compile unless File.exists?(builder.stylesheet_fullpath)
href = builder.stylesheet_path(current_hostname)
cache.defer_set("path_#{target}_#{theme_id}_#{current_hostname}", href)
if is_theme && !has_theme
next
else
next if is_theme && builder.theme&.component && !scss_checker.has_scss(theme_id)
builder.compile unless File.exists?(builder.stylesheet_fullpath)
href = builder.stylesheet_path(current_hostname)
cache.defer_set("path_#{target}_#{theme_id}_#{current_hostname}", href)
end
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)
data[:new_href] = href
cache.defer_set("path_#{target}__#{current_hostname}", href)
data = { target: target, new_href: href }
stylesheets << data
end

View File

@ -117,7 +117,7 @@ class Stylesheet::Manager::Builder
if is_theme?
"#{@target}_#{theme&.id}"
elsif @color_scheme
"#{@target}_#{scheme_slug}_#{@color_scheme&.id.to_s}"
"#{@target}_#{scheme_slug}_#{@color_scheme&.id.to_s}_#{@theme&.id}"
else
scheme_string = theme&.color_scheme ? "_#{theme.color_scheme.id}" : ""
"#{@target}#{scheme_string}"
@ -137,6 +137,10 @@ 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
@ -146,8 +150,10 @@ class Stylesheet::Manager::Builder
@digest ||= begin
if is_theme?
theme_digest
else
elsif is_color_scheme?
color_scheme_digest
else
default_digest
end
end
end
@ -171,7 +177,7 @@ class Stylesheet::Manager::Builder
end
def theme_digest
Digest::SHA1.hexdigest(scss_digest.to_s + color_scheme_digest.to_s + settings_digest + plugins_digest + uploads_digest)
Digest::SHA1.hexdigest(scss_digest.to_s + color_scheme_digest.to_s + settings_digest + uploads_digest)
end
# this protects us from situations where new versions of a plugin removed a file
@ -218,6 +224,10 @@ 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

View File

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

View File

@ -45,13 +45,13 @@ task 'assets:precompile:css' => 'environment' do
STDERR.puts "Start compiling CSS: #{Time.zone.now}"
RailsMultisite::ConnectionManagement.each_connection do |db|
next if ENV["PRECOMPILE_SHARED_MULTISITE_CSS"] == "1" && db != "default"
# css will get precompiled during first request if tables do not exist.
# CSS will get precompiled during first request if tables do not exist.
if ActiveRecord::Base.connection.table_exists?(Theme.table_name)
STDERR.puts "Compiling css for #{db} #{Time.zone.now}"
STDERR.puts "-------------"
STDERR.puts "Compiling CSS for #{db} #{Time.zone.now}"
begin
Stylesheet::Manager.precompile_css
Stylesheet::Manager.precompile_css if db == "default"
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."

View File

@ -11,19 +11,16 @@ describe Stylesheet::Importer do
context "#category_backgrounds" do
it "applies CDN to background category images" do
expect(compile_css("mobile")).to_not include("body.category-")
expect(compile_css("desktop")).to_not include("body.category-")
expect(compile_css("color_definitions")).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("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})}")
expect(compile_css("color_definitions")).to include("body.category-#{parent_category.slug}-#{category.slug}{background-image:url(#{background.url})}")
GlobalSetting.stubs(:cdn_url).returns("//awesome.cdn")
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})}")
expect(compile_css("color_definitions")).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
@ -36,8 +33,7 @@ describe Stylesheet::Importer do
background = Fabricate(:upload_s3)
category = Fabricate(:category, uploaded_background: background)
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")
expect(compile_css("color_definitions")).to include("body.category-#{category.slug}{background-image:url(https://s3.cdn/original")
end
end
@ -45,8 +41,7 @@ describe Stylesheet::Importer do
context "#font" do
it "includes font variable" do
default_font = ":root{--font-family: Arial, sans-serif}"
expect(compile_css("desktop")).to include(default_font)
expect(compile_css("mobile")).to include(default_font)
expect(compile_css("color_definitions")).to include(default_font)
expect(compile_css("embed")).to include(default_font)
expect(compile_css("publish")).to include(default_font)
end
@ -58,14 +53,14 @@ describe Stylesheet::Importer do
SiteSetting.base_font = base_font[:key]
SiteSetting.heading_font = heading_font[:key]
expect(compile_css("desktop"))
expect(compile_css("color_definitions"))
.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("mobile"))
expect(compile_css("color_definitions"))
.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

View File

@ -39,7 +39,7 @@ describe Stylesheet::Manager do
}}
it "generates the right links for non-theme targets" do
manager = manager(theme.id)
manager = manager(nil)
hrefs = manager.stylesheet_details(:desktop, 'all')
@ -188,22 +188,12 @@ describe Stylesheet::Manager do
DiscoursePluginRegistry.reset!
end
it 'can correctly account for plugins in digest' do
theme = Fabricate(:theme)
manager = manager(theme.id)
builder = Stylesheet::Manager::Builder.new(
target: :desktop_theme, theme: theme, manager: manager
)
it 'can correctly account for plugins in default digest' do
builder = Stylesheet::Manager::Builder.new(target: :desktop, manager: manager)
digest1 = builder.digest
DiscoursePluginRegistry.stylesheets["fake"] = Set.new(["fake_file"])
builder = Stylesheet::Manager::Builder.new(
target: :desktop_theme, theme: theme, manager: manager
)
builder = Stylesheet::Manager::Builder.new(target: :desktop, manager: manager)
digest2 = builder.digest
expect(digest1).not_to eq(digest2)
@ -282,6 +272,21 @@ 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
@ -497,6 +502,15 @@ 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;}')
@ -602,8 +616,7 @@ describe Stylesheet::Manager do
end
end
# this test takes too long, we don't run it by default
describe ".precompile_css", if: ENV["RUN_LONG_TESTS"] == "1" do
describe ".precompile css" do
before do
class << STDERR
alias_method :orig_write, :write
@ -626,7 +639,6 @@ describe Stylesheet::Manager do
scheme2 = ColorScheme.create!(name: "scheme2")
core_targets = [:desktop, :mobile, :desktop_rtl, :mobile_rtl, :admin, :wizard]
theme_targets = [:desktop_theme, :mobile_theme]
color_scheme_targets = ["color_definitions_scheme1_#{scheme1.id}", "color_definitions_scheme2_#{scheme2.id}"]
Theme.update_all(user_selectable: false)
user_theme = Fabricate(:theme, user_selectable: true, color_scheme: scheme1)
@ -657,19 +669,30 @@ describe Stylesheet::Manager do
StylesheetCache.destroy_all
# only core
output = capture_output(:stderr) do
Stylesheet::Manager.precompile_css
end
results = StylesheetCache.pluck(:target)
expect(results.size).to eq(core_targets.size)
StylesheetCache.destroy_all
# only themes
output = capture_output(:stderr) do
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(16) # (3 themes * 2 targets) + 10 color schemes (2 themes * 5 color schemes (4 defaults + 1 theme scheme))
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)
core_targets.each do |tar|
expect(results.count { |target| target =~ /^#{tar}_(#{scheme1.id}|#{scheme2.id})$/ }).to eq(2)
end
# themes + core
Stylesheet::Manager.precompile_css
results = StylesheetCache.pluck(:target)
expect(results.size).to eq(22) # 6 core targets + 6 theme + 10 color schemes
theme_targets.each do |tar|
expect(results.count { |target| target =~ /^#{tar}_(#{user_theme.id}|#{default_theme.id})$/ }).to eq(2)
@ -678,21 +701,14 @@ 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(22) # 6 core targets + 6 theme + 10 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])
expect(results).to include("color_definitions_#{scheme1.name}_#{scheme1.id}_#{user_theme.id}")
expect(results).to include("color_definitions_#{scheme2.name}_#{scheme2.id}_#{default_theme.id}")
end
end
end