From cd24eff5d98e74db4afe0257e4d9de1ad2556c5d Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Mon, 12 Apr 2021 15:02:58 +0300 Subject: [PATCH] FEATURE: Introduce theme/component QUnit tests (take 2) (#12661) This commit allows themes and theme components to have QUnit tests. To add tests to your theme/component, create a top-level directory in your theme and name it `test`, and Discourse will save all the files in that directory (and its sub-directories) as "tests files" in the database. While tests files/directories are not required to be organized in a specific way, we recommend that you follow Discourse core's tests [structure](https://github.com/discourse/discourse/tree/master/app/assets/javascripts/discourse/tests). Writing theme tests should be identical to writing plugins or core tests; all the `import` statements and APIs that you see in core (or plugins) to define/setup tests should just work in themes. You do need a working Discourse install to run theme tests, and you have 2 ways to run theme tests: * In the browser at the `/qunit` route. `/qunit` will run tests of all active themes/components as well as core and plugins. The `/qunit` now accepts a `theme_name` or `theme_url` params that you can use to run tests of a specific theme/component like so: `/qunit?theme_name=`. * In the command line using the `themes:qunit` rake task. This take is meant to run tests of a single theme/component so you need to provide it with a theme name or URL like so: `bundle exec rake themes:qunit[name=]` or `bundle exec rake themes:qunit[url=]`. There are some refactors to how Discourse processes JavaScript that comes with themes/components, and these refactors may break your JS customizations; see https://meta.discourse.org/t/upcoming-core-changes-that-may-break-some-themes-components-april-12/186252?u=osama for details on how you can check if your themes/components are affected and what you need to do to fix them. This commit also improves theme error handling in Discourse. We will now be able to catch errors that occur when theme initializers are run and prevent them from breaking the site and other themes/components. --- app/assets/javascripts/discourse/app/app.js | 61 ++++++++- .../discourse/app/helpers/theme-helpers.js | 6 +- .../app/initializers/auto-load-modules.js | 1 - .../discourse/app/lib/theme-settings-store.js | 47 +++++++ .../discourse/app/lib/utilities.js | 35 ----- .../pre-initializers/theme-errors-handler.js | 56 ++++++++ .../discourse/app/services/theme-settings.js | 26 ---- .../discourse/tests/setup-tests.js | 18 ++- .../discourse/tests/test_helper.js | 10 -- .../discourse/tests/test_starter.js | 11 ++ app/controllers/application_controller.rb | 7 + app/controllers/qunit_controller.rb | 17 ++- .../theme_javascripts_controller.rb | 25 +++- app/helpers/application_helper.rb | 21 ++- app/helpers/qunit_helper.rb | 24 ++++ app/models/theme.rb | 8 +- app/models/theme_field.rb | 41 +++++- app/views/qunit/index.html.erb | 5 + config/routes.rb | 1 + lib/tasks/qunit.rake | 2 +- lib/tasks/themes.rake | 21 +++ lib/theme_javascript_compiler.rb | 66 +++++----- spec/lib/theme_javascript_compiler_spec.rb | 14 ++ spec/models/remote_theme_spec.rb | 6 +- spec/models/theme_field_spec.rb | 28 ++-- spec/models/theme_spec.rb | 120 +++++------------- spec/requests/qunit_controller_spec.rb | 76 +++++++++++ .../theme_javascripts_controller_spec.rb | 20 +++ 28 files changed, 527 insertions(+), 246 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/theme-settings-store.js create mode 100644 app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js delete mode 100644 app/assets/javascripts/discourse/app/services/theme-settings.js create mode 100644 app/assets/javascripts/discourse/tests/test_starter.js create mode 100644 app/helpers/qunit_helper.rb create mode 100644 spec/requests/qunit_controller_spec.rb diff --git a/app/assets/javascripts/discourse/app/app.js b/app/assets/javascripts/discourse/app/app.js index b28effdddb4..ddcc134df98 100644 --- a/app/assets/javascripts/discourse/app/app.js +++ b/app/assets/javascripts/discourse/app/app.js @@ -1,8 +1,10 @@ import Application from "@ember/application"; import Mousetrap from "mousetrap"; import { buildResolver } from "discourse-common/resolver"; +import { isTesting } from "discourse-common/config/environment"; const _pluginCallbacks = []; +let _themeErrors = []; const Discourse = Application.extend({ rootElement: "#main", @@ -19,14 +21,37 @@ const Discourse = Application.extend({ Resolver: buildResolver("discourse"), _prepareInitializer(moduleName) { - const module = requirejs(moduleName, null, null, true); - if (!module) { - throw new Error(moduleName + " must export an initializer."); + const themeId = moduleThemeId(moduleName); + let module = null; + + try { + module = requirejs(moduleName, null, null, true); + + if (!module) { + throw new Error(moduleName + " must export an initializer."); + } + } catch (err) { + if (!themeId || isTesting()) { + throw err; + } + _themeErrors.push([themeId, err]); + fireThemeErrorEvent(); + return; } const init = module.default; const oldInitialize = init.initialize; - init.initialize = (app) => oldInitialize.call(init, app.__container__, app); + init.initialize = (app) => { + try { + return oldInitialize.call(init, app.__container__, app); + } catch (err) { + if (!themeId || isTesting()) { + throw err; + } + _themeErrors.push([themeId, err]); + fireThemeErrorEvent(); + } + }; return init; }, @@ -37,9 +62,15 @@ const Discourse = Application.extend({ Object.keys(requirejs._eak_seen).forEach((key) => { if (/\/pre\-initializers\//.test(key)) { - this.initializer(this._prepareInitializer(key)); + const initializer = this._prepareInitializer(key); + if (initializer) { + this.initializer(initializer); + } } else if (/\/(api\-)?initializers\//.test(key)) { - this.instanceInitializer(this._prepareInitializer(key)); + const initializer = this._prepareInitializer(key); + if (initializer) { + this.instanceInitializer(initializer); + } } }); @@ -60,4 +91,22 @@ const Discourse = Application.extend({ }, }); +function moduleThemeId(moduleName) { + const match = moduleName.match(/^discourse\/theme\-(\d+)\//); + if (match) { + return parseInt(match[1], 10); + } +} + +function fireThemeErrorEvent() { + const event = new CustomEvent("discourse-theme-error"); + document.dispatchEvent(event); +} + +export function getAndClearThemeErrors() { + const copy = _themeErrors; + _themeErrors = []; + return copy; +} + export default Discourse; diff --git a/app/assets/javascripts/discourse/app/helpers/theme-helpers.js b/app/assets/javascripts/discourse/app/helpers/theme-helpers.js index 989877554f1..477b89b1b06 100644 --- a/app/assets/javascripts/discourse/app/helpers/theme-helpers.js +++ b/app/assets/javascripts/discourse/app/helpers/theme-helpers.js @@ -1,6 +1,7 @@ -import { helperContext, registerUnbound } from "discourse-common/lib/helpers"; +import { registerUnbound } from "discourse-common/lib/helpers"; import I18n from "I18n"; import deprecated from "discourse-common/lib/deprecated"; +import { getSetting as getThemeSetting } from "discourse/lib/theme-settings-store"; registerUnbound("theme-i18n", (themeId, key, params) => { return I18n.t(`theme_translations.${themeId}.${key}`, params); @@ -18,5 +19,6 @@ registerUnbound("theme-setting", (themeId, key, hash) => { { since: "v2.2.0.beta8", dropFrom: "v2.3.0" } ); } - return helperContext().themeSettings.getSetting(themeId, key); + + return getThemeSetting(themeId, key); }); diff --git a/app/assets/javascripts/discourse/app/initializers/auto-load-modules.js b/app/assets/javascripts/discourse/app/initializers/auto-load-modules.js index b985d85c372..fa2ca51902a 100644 --- a/app/assets/javascripts/discourse/app/initializers/auto-load-modules.js +++ b/app/assets/javascripts/discourse/app/initializers/auto-load-modules.js @@ -19,7 +19,6 @@ export function autoLoadModules(container, registry) { let context = { siteSettings: container.lookup("site-settings:main"), - themeSettings: container.lookup("service:theme-settings"), keyValueStore: container.lookup("key-value-store:main"), capabilities: container.lookup("capabilities:main"), currentUser: container.lookup("current-user:main"), diff --git a/app/assets/javascripts/discourse/app/lib/theme-settings-store.js b/app/assets/javascripts/discourse/app/lib/theme-settings-store.js new file mode 100644 index 00000000000..6503d6b932c --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/theme-settings-store.js @@ -0,0 +1,47 @@ +import { get } from "@ember/object"; + +const originalSettings = {}; +const settings = {}; + +export function registerSettings( + themeId, + settingsObject, + { force = false } = {} +) { + if (settings[themeId] && !force) { + return; + } + originalSettings[themeId] = Object.assign({}, settingsObject); + const s = {}; + Object.keys(settingsObject).forEach((key) => { + Object.defineProperty(s, key, { + enumerable: true, + get() { + return settingsObject[key]; + }, + set(newVal) { + settingsObject[key] = newVal; + }, + }); + }); + settings[themeId] = s; +} + +export function getSetting(themeId, settingKey) { + if (settings[themeId]) { + return get(settings[themeId], settingKey); + } + return null; +} + +export function getObjectForTheme(themeId) { + return settings[themeId]; +} + +export function resetSettings() { + Object.keys(originalSettings).forEach((themeId) => { + Object.keys(originalSettings[themeId]).forEach((key) => { + settings[themeId][key] = originalSettings[themeId][key]; + }); + }); +} diff --git a/app/assets/javascripts/discourse/app/lib/utilities.js b/app/assets/javascripts/discourse/app/lib/utilities.js index f7b7bad13e8..c635edcc2bb 100644 --- a/app/assets/javascripts/discourse/app/lib/utilities.js +++ b/app/assets/javascripts/discourse/app/lib/utilities.js @@ -1,6 +1,5 @@ import getURL, { getURLWithCDN } from "discourse-common/lib/get-url"; import Handlebars from "handlebars"; -import I18n from "I18n"; import { deepMerge } from "discourse-common/lib/object"; import { escape } from "pretty-text/sanitizer"; import { helperContext } from "discourse-common/lib/helpers"; @@ -444,40 +443,6 @@ export function postRNWebviewMessage(prop, value) { } } -function reportToLogster(name, error) { - const data = { - message: `${name} theme/component is throwing errors`, - stacktrace: error.stack, - }; - - Ember.$.ajax(getURL("/logs/report_js_error"), { - data, - type: "POST", - cache: false, - }); -} -// this function is used in lib/theme_javascript_compiler.rb -export function rescueThemeError(name, error, api) { - /* eslint-disable-next-line no-console */ - console.error(`"${name}" error:`, error); - reportToLogster(name, error); - - const currentUser = api.getCurrentUser(); - if (!currentUser || !currentUser.admin) { - return; - } - - const path = getURL(`/admin/customize/themes`); - const message = I18n.t("themes.broken_theme_alert", { - theme: name, - path: `${path}`, - }); - const alertDiv = document.createElement("div"); - alertDiv.classList.add("broken-theme-alert"); - alertDiv.innerHTML = `⚠️ ${message}`; - document.body.prepend(alertDiv); -} - const CODE_BLOCKS_REGEX = /^( |\t).*|`[^`]+`|^```[^]*?^```|\[code\][^]*?\[\/code\]/gm; // | ^ | ^ | ^ | ^ | // | | | | diff --git a/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js b/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js new file mode 100644 index 00000000000..05a9a0e153f --- /dev/null +++ b/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js @@ -0,0 +1,56 @@ +import { isTesting } from "discourse-common/config/environment"; +import { getAndClearThemeErrors } from "discourse/app"; +import PreloadStore from "discourse/lib/preload-store"; +import getURL from "discourse-common/lib/get-url"; +import I18n from "I18n"; + +export default { + name: "theme-errors-handler", + after: "inject-discourse-objects", + + initialize(container) { + const currentUser = container.lookup("current-user:main"); + if (isTesting()) { + return; + } + renderErrorNotices(currentUser); + document.addEventListener("discourse-theme-error", () => + renderErrorNotices(currentUser) + ); + }, +}; + +function reportToLogster(name, error) { + const data = { + message: `${name} theme/component is throwing errors`, + stacktrace: error.stack, + }; + + Ember.$.ajax(getURL("/logs/report_js_error"), { + data, + type: "POST", + cache: false, + }); +} + +function renderErrorNotices(currentUser) { + getAndClearThemeErrors().forEach(([themeId, error]) => { + const name = + PreloadStore.get("activatedThemes")[themeId] || `(theme-id: ${themeId})`; + /* eslint-disable-next-line no-console */ + console.error(`An error occurred in the "${name}" theme/component:`, error); + reportToLogster(name, error); + if (!currentUser || !currentUser.admin) { + return; + } + const path = getURL("/admin/customize/themes"); + const message = I18n.t("themes.broken_theme_alert", { + theme: name, + path: `${path}`, + }); + const alertDiv = document.createElement("div"); + alertDiv.classList.add("broken-theme-alert"); + alertDiv.innerHTML = `⚠️ ${message}`; + document.body.prepend(alertDiv); + }); +} diff --git a/app/assets/javascripts/discourse/app/services/theme-settings.js b/app/assets/javascripts/discourse/app/services/theme-settings.js deleted file mode 100644 index 3a1afbdf223..00000000000 --- a/app/assets/javascripts/discourse/app/services/theme-settings.js +++ /dev/null @@ -1,26 +0,0 @@ -import Service from "@ember/service"; -import { get } from "@ember/object"; - -export default Service.extend({ - settings: null, - - init() { - this._super(...arguments); - this._settings = {}; - }, - - registerSettings(themeId, settingsObject) { - this._settings[themeId] = settingsObject; - }, - - getSetting(themeId, settingsKey) { - if (this._settings[themeId]) { - return get(this._settings[themeId], settingsKey); - } - return null; - }, - - getObjectForTheme(themeId) { - return this._settings[themeId]; - }, -}); diff --git a/app/assets/javascripts/discourse/tests/setup-tests.js b/app/assets/javascripts/discourse/tests/setup-tests.js index 066d4ff314f..df1c06c3939 100644 --- a/app/assets/javascripts/discourse/tests/setup-tests.js +++ b/app/assets/javascripts/discourse/tests/setup-tests.js @@ -17,6 +17,7 @@ import { setupS3CDN, setupURL } from "discourse-common/lib/get-url"; import Application from "../app"; import MessageBus from "message-bus-client"; import PreloadStore from "discourse/lib/preload-store"; +import { resetSettings as resetThemeSettings } from "discourse/lib/theme-settings-store"; import QUnit from "qunit"; import { ScrollingDOMMethods } from "discourse/mixins/scrolling"; import Session from "discourse/models/session"; @@ -154,6 +155,7 @@ function setupTestsCommon(application, container, config) { QUnit.testStart(function (ctx) { bootbox.$body = $("#ember-testing"); let settings = resetSettings(); + resetThemeSettings(); if (config) { // Ember CLI testing environment @@ -251,6 +253,8 @@ function setupTestsCommon(application, container, config) { let pluginPath = getUrlParameter("qunit_single_plugin") ? "/" + getUrlParameter("qunit_single_plugin") + "/" : "/plugins/"; + let themeOnly = getUrlParameter("theme_name") || getUrlParameter("theme_url"); + if (getUrlParameter("qunit_disable_auto_start") === "1") { QUnit.config.autostart = false; } @@ -259,8 +263,20 @@ function setupTestsCommon(application, container, config) { let isTest = /\-test/.test(entry); let regex = new RegExp(pluginPath); let isPlugin = regex.test(entry); + let isTheme = /^discourse\/theme\-\d+\/.+/.test(entry); - if (isTest && (!skipCore || isPlugin)) { + if (!isTest) { + return; + } + + if (themeOnly) { + if (isTheme) { + require(entry, null, null, true); + } + return; + } + + if (!skipCore || isPlugin) { require(entry, null, null, true); } }); diff --git a/app/assets/javascripts/discourse/tests/test_helper.js b/app/assets/javascripts/discourse/tests/test_helper.js index 595e95ecaaf..affd1b36ee0 100644 --- a/app/assets/javascripts/discourse/tests/test_helper.js +++ b/app/assets/javascripts/discourse/tests/test_helper.js @@ -41,13 +41,3 @@ //= require setup-tests //= require test-shims //= require jquery.magnific-popup.min.js - -document.write( - '
' -); -document.write( - "" -); - -let setupTestsLegacy = require("discourse/tests/setup-tests").setupTestsLegacy; -setupTestsLegacy(window.Discourse); diff --git a/app/assets/javascripts/discourse/tests/test_starter.js b/app/assets/javascripts/discourse/tests/test_starter.js new file mode 100644 index 00000000000..77b69af2a10 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/test_starter.js @@ -0,0 +1,11 @@ +// discourse-skip-module + +document.write( + '
' +); +document.write( + "" +); + +let setupTestsLegacy = require("discourse/tests/setup-tests").setupTestsLegacy; +setupTestsLegacy(window.Discourse); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7fc969b31a3..885e53e91ad 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -570,6 +570,7 @@ class ApplicationController < ActionController::Base store_preloaded("banner", banner_json) store_preloaded("customEmoji", custom_emoji) store_preloaded("isReadOnly", @readonly_mode.to_s) + store_preloaded("activatedThemes", activated_themes_json) end def preload_current_user_data @@ -890,4 +891,10 @@ class ApplicationController < ActionController::Base end end + def activated_themes_json + ids = @theme_ids&.compact + return "{}" if ids.blank? + ids = Theme.transform_ids(ids) + Theme.where(id: ids).pluck(:id, :name).to_h.to_json + end end diff --git a/app/controllers/qunit_controller.rb b/app/controllers/qunit_controller.rb index f042d0a634d..5e71847d801 100644 --- a/app/controllers/qunit_controller.rb +++ b/app/controllers/qunit_controller.rb @@ -1,11 +1,26 @@ # frozen_string_literal: true class QunitController < ApplicationController - skip_before_action :check_xhr, :preload_json, :redirect_to_login_if_required + skip_before_action *%i{ + check_xhr + preload_json + redirect_to_login_if_required + } layout false # only used in test / dev def index raise Discourse::InvalidAccess.new if Rails.env.production? + if (theme_name = params[:theme_name]).present? + theme = Theme.find_by(name: theme_name) + raise Discourse::NotFound if theme.blank? + elsif (theme_url = params[:theme_url]).present? + theme = RemoteTheme.find_by(remote_url: theme_url) + raise Discourse::NotFound if theme.blank? + end + if theme.present? + request.env[:resolved_theme_ids] = [theme.id] + request.env[:skip_theme_ids_transformation] = true + end end end diff --git a/app/controllers/theme_javascripts_controller.rb b/app/controllers/theme_javascripts_controller.rb index 25dfd6567db..e28d0ed58f5 100644 --- a/app/controllers/theme_javascripts_controller.rb +++ b/app/controllers/theme_javascripts_controller.rb @@ -8,7 +8,7 @@ class ThemeJavascriptsController < ApplicationController :preload_json, :redirect_to_login_if_required, :verify_authenticity_token, - only: [:show] + only: [:show, :show_tests] ) before_action :is_asset_path, :no_cookies, :apply_cdn_headers, only: [:show] @@ -34,6 +34,29 @@ class ThemeJavascriptsController < ApplicationController send_file(cache_file, disposition: :inline) end + def show_tests + raise Discourse::NotFound if Rails.env.production? + + theme_id = params.require(:theme_id) + theme = Theme.find(theme_id) + content = ThemeField + .where( + theme_id: theme_id, + target_id: Theme.targets[:tests_js] + ) + .each(&:ensure_baked!) + .map(&:value_baked) + .join("\n") + + ThemeJavascriptCompiler.force_default_settings(content, theme) + + response.headers["Content-Length"] = content.size.to_s + response.headers["Last-Modified"] = Time.zone.now.httpdate + immutable_for(1.second) + + send_data content, filename: "js-tests-theme-#{theme_id}.js", disposition: :inline + end + private def query diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index b27b889c5be..0e53e2788bd 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -453,15 +453,30 @@ module ApplicationHelper end def theme_lookup(name) - Theme.lookup_field(theme_ids, mobile_view? ? :mobile : :desktop, name) + Theme.lookup_field( + theme_ids, + mobile_view? ? :mobile : :desktop, + name, + skip_transformation: request.env[:skip_theme_ids_transformation].present? + ) end def theme_translations_lookup - Theme.lookup_field(theme_ids, :translations, I18n.locale) + Theme.lookup_field( + theme_ids, + :translations, + I18n.locale, + skip_transformation: request.env[:skip_theme_ids_transformation].present? + ) end def theme_js_lookup - Theme.lookup_field(theme_ids, :extra_js, nil) + Theme.lookup_field( + theme_ids, + :extra_js, + nil, + skip_transformation: request.env[:skip_theme_ids_transformation].present? + ) end def discourse_stylesheet_link_tag(name, opts = {}) diff --git a/app/helpers/qunit_helper.rb b/app/helpers/qunit_helper.rb new file mode 100644 index 00000000000..bf7955a6d93 --- /dev/null +++ b/app/helpers/qunit_helper.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module QunitHelper + def theme_tests + theme_ids = request.env[:resolved_theme_ids] + return "" if theme_ids.blank? + + skip_transformation = request.env[:skip_theme_ids_transformation] + query = ThemeField + .joins(:theme) + .where( + target_id: Theme.targets[:tests_js], + theme_id: skip_transformation ? theme_ids : Theme.transform_ids(theme_ids) + ) + .pluck(:theme_id) + .uniq + .map do |theme_id| + src = "#{GlobalSetting.cdn_url}#{Discourse.base_path}/theme-javascripts/tests/#{theme_id}.js" + "" + end + .join("\n") + .html_safe + end +end diff --git a/app/models/theme.rb b/app/models/theme.rb index 94972fb2b15..63ab1024822 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -128,7 +128,7 @@ class Theme < ActiveRecord::Base SvgSprite.expire_cache end - BASE_COMPILER_VERSION = 17 + BASE_COMPILER_VERSION = 48 def self.compiler_version get_set_cache "compiler_version" do dependencies = [ @@ -262,11 +262,11 @@ class Theme < ActiveRecord::Base end end - def self.lookup_field(theme_ids, target, field) + def self.lookup_field(theme_ids, target, field, skip_transformation: false) return if theme_ids.blank? theme_ids = [theme_ids] unless Array === theme_ids - theme_ids = transform_ids(theme_ids) + theme_ids = transform_ids(theme_ids) if !skip_transformation cache_key = "#{theme_ids.join(",")}:#{target}:#{field}:#{Theme.compiler_version}" lookup = @cache[cache_key] return lookup.html_safe if lookup @@ -297,7 +297,7 @@ class Theme < ActiveRecord::Base end def self.targets - @targets ||= Enum.new(common: 0, desktop: 1, mobile: 2, settings: 3, translations: 4, extra_scss: 5, extra_js: 6) + @targets ||= Enum.new(common: 0, desktop: 1, mobile: 2, settings: 3, translations: 4, extra_scss: 5, extra_js: 6, tests_js: 7) end def self.lookup_target(target_id) diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 673386b791b..ab3d96d263a 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -94,10 +94,32 @@ class ThemeField < ActiveRecord::Base node.remove end - doc.css('script[type="text/discourse-plugin"]').each do |node| - next unless node['version'].present? + doc.css('script[type="text/discourse-plugin"]').each_with_index do |node, index| + version = node['version'] + next if version.blank? + + initializer_name = "theme-field" + + "-#{self.id}" + + "-#{Theme.targets[self.target_id]}" + + "-#{ThemeField.types[self.type_id]}" + + "-script-#{index + 1}" begin - js_compiler.append_plugin_script(node.inner_html, node['version']) + js = <<~JS + import { withPluginApi } from "discourse/lib/plugin-api"; + + export default { + name: #{initializer_name.inspect}, + after: "inject-objects", + + initialize() { + withPluginApi(#{version.inspect}, (api) => { + #{node.inner_html} + }); + } + }; + JS + + js_compiler.append_module(js, "discourse/initializers/#{initializer_name}", include_variables: true) rescue ThemeJavascriptCompiler::CompileError => ex errors << ex.message end @@ -132,7 +154,7 @@ class ThemeField < ActiveRecord::Base begin case extension when "js.es6", "js" - js_compiler.append_module(content, filename) + js_compiler.append_module(content, filename, include_variables: true) when "hbs" js_compiler.append_ember_template(filename.sub("discourse/templates/", ""), content) when "hbr", "raw.hbs" @@ -285,6 +307,10 @@ class ThemeField < ActiveRecord::Base Theme.targets[self.target_id] == :extra_js end + def js_tests_field? + Theme.targets[self.target_id] == :tests_js + end + def basic_scss_field? ThemeField.basic_targets.include?(Theme.targets[self.target_id].to_s) && ThemeField.scss_fields.include?(self.name) @@ -315,7 +341,7 @@ class ThemeField < ActiveRecord::Base self.error = nil unless self.error.present? self.compiler_version = Theme.compiler_version DB.after_commit { CSP::Extension.clear_theme_extensions_cache! } - elsif extra_js_field? + elsif extra_js_field? || js_tests_field? self.value_baked, self.error = process_extra_js(self.value) self.error = nil unless self.error.present? self.compiler_version = Theme.compiler_version @@ -422,7 +448,7 @@ class ThemeField < ActiveRecord::Base hash = {} OPTIONS.each do |option| plural = :"#{option}s" - hash[option] = @allowed_values[plural][0] if @allowed_values[plural] && @allowed_values[plural].length == 1 + hash[option] = @allowed_values[plural][0] if @allowed_values[plural]&.length == 1 hash[option] = match[option] if hash[option].nil? end hash @@ -457,6 +483,9 @@ class ThemeField < ActiveRecord::Base ThemeFileMatcher.new(regex: /^javascripts\/(?.+)$/, targets: :extra_js, names: nil, types: :js, canonical: -> (h) { "javascripts/#{h[:name]}" }), + ThemeFileMatcher.new(regex: /^test\/(?.+)$/, + targets: :tests_js, names: nil, types: :js, + canonical: -> (h) { "test/#{h[:name]}" }), ThemeFileMatcher.new(regex: /^settings\.ya?ml$/, names: "yaml", types: :yaml, targets: :settings, canonical: -> (h) { "settings.yml" }), diff --git a/app/views/qunit/index.html.erb b/app/views/qunit/index.html.erb index 9b116c83dd5..a8b57ce32b8 100644 --- a/app/views/qunit/index.html.erb +++ b/app/views/qunit/index.html.erb @@ -5,6 +5,11 @@ <%= discourse_color_scheme_stylesheets %> <%= stylesheet_link_tag "test_helper" %> <%= javascript_include_tag "test_helper" %> + <%= theme_tests %> + <%= theme_translations_lookup %> + <%= theme_js_lookup %> + <%= theme_lookup("head_tag") %> + <%= javascript_include_tag "test_starter" %> <%= csrf_meta_tags %> diff --git a/config/routes.rb b/config/routes.rb index b92a8e2e167..3374809d6d7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -527,6 +527,7 @@ Discourse::Application.routes.draw do get "stylesheets/:name.css" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/ } get "color-scheme-stylesheet/:id(/:theme_id)" => "stylesheets#color_scheme", constraints: { format: :json } get "theme-javascripts/:digest.js" => "theme_javascripts#show", constraints: { digest: /\h{40}/ } + get "theme-javascripts/tests/:theme_id.js" => "theme_javascripts#show_tests" post "uploads/lookup-metadata" => "uploads#metadata" post "uploads" => "uploads#create" diff --git a/lib/tasks/qunit.rake b/lib/tasks/qunit.rake index 405e932cc64..1cb656993eb 100644 --- a/lib/tasks/qunit.rake +++ b/lib/tasks/qunit.rake @@ -61,7 +61,7 @@ task "qunit:test", [:timeout, :qunit_path] do |_, args| cmd = "node #{test_path}/run-qunit.js http://localhost:#{port}#{qunit_path}" options = { seed: (ENV["QUNIT_SEED"] || Random.new.seed), hidepassed: 1 } - %w{module filter qunit_skip_core qunit_single_plugin}.each do |arg| + %w{module filter qunit_skip_core qunit_single_plugin theme_name theme_url}.each do |arg| options[arg] = ENV[arg.upcase] if ENV[arg.upcase].present? end diff --git a/lib/tasks/themes.rake b/lib/tasks/themes.rake index 05674767466..e2912ec0577 100644 --- a/lib/tasks/themes.rake +++ b/lib/tasks/themes.rake @@ -96,3 +96,24 @@ task "themes:audit" => :environment do puts repo end end + +desc "Run QUnit tests of a theme/component" +task "themes:qunit", :theme_name_or_url do |t, args| + name_or_url = args[:theme_name_or_url] + if name_or_url.blank? + raise "A theme name or URL must be provided." + end + if name_or_url =~ /^(url|name)=(.+)/ + cmd = "THEME_#{Regexp.last_match(1).upcase}=#{Regexp.last_match(2)} " + cmd += `which rake`.strip + " qunit:test" + sh cmd + else + raise <<~MSG + Cannot parse passed argument #{name_or_url.inspect}. + Usage: + `bundle exec rake themes:unit[url=]` + OR + `bundle exec rake themes:unit[name=]` + MSG + end +end diff --git a/lib/theme_javascript_compiler.rb b/lib/theme_javascript_compiler.rb index 17bebf1b229..e9c545945e1 100644 --- a/lib/theme_javascript_compiler.rb +++ b/lib/theme_javascript_compiler.rb @@ -151,6 +151,18 @@ class ThemeJavascriptCompiler class CompileError < StandardError end + def self.force_default_settings(content, theme) + settings_hash = {} + theme.settings.each do |setting| + settings_hash[setting.name] = setting.default + end + content.prepend <<~JS + (function() { + require("discourse/lib/theme-settings-store").registerSettings(#{theme.id}, #{settings_hash.to_json}, { force: true }); + })(); + JS + end + attr_accessor :content def initialize(theme_id, theme_name) @@ -162,10 +174,8 @@ class ThemeJavascriptCompiler def prepend_settings(settings_hash) @content.prepend <<~JS (function() { - if ('Discourse' in window && Discourse.__container__) { - Discourse.__container__ - .lookup("service:theme-settings") - .registerSettings(#{@theme_id}, #{settings_hash.to_json}); + if ('require' in window) { + require("discourse/lib/theme-settings-store").registerSettings(#{@theme_id}, #{settings_hash.to_json}); } })(); JS @@ -173,8 +183,14 @@ class ThemeJavascriptCompiler # TODO Error handling for handlebars templates def append_ember_template(name, hbs_template) + if !name.start_with?("javascripts/") + prefix = "javascripts" + prefix += "/" if !name.start_with?("/") + name = prefix + name + end name = name.inspect compiled = EmberTemplatePrecompiler.new(@theme_id).compile(hbs_template) + # the `'Ember' in window` check is needed for no_ember pages content << <<~JS (function() { if ('Ember' in window) { @@ -204,18 +220,19 @@ class ThemeJavascriptCompiler raise CompileError.new e.instance_variable_get(:@error) # e.message contains the entire template, which could be very long end - def append_plugin_script(script, api_version) - @content << transpile(script, api_version) - end - def append_raw_script(script) @content << script + "\n" end def append_module(script, name, include_variables: true) - script = "#{theme_variables}#{script}" if include_variables + name = "discourse/theme-#{@theme_id}/#{name.gsub(/^discourse\//, '')}" + script = "#{theme_settings}#{script}" if include_variables transpiler = DiscourseJsProcessor::Transpiler.new - @content << transpiler.perform(script, "", name) + @content << <<~JS + if ('define' in window) { + #{transpiler.perform(script, "", name).strip} + } + JS rescue MiniRacer::RuntimeError => ex raise CompileError.new ex.message end @@ -226,36 +243,11 @@ class ThemeJavascriptCompiler private - def theme_variables + def theme_settings <<~JS - const __theme_name__ = "#{@theme_name.gsub('"', "\\\"")}"; - const settings = Discourse.__container__ - .lookup("service:theme-settings") + const settings = require("discourse/lib/theme-settings-store") .getObjectForTheme(#{@theme_id}); const themePrefix = (key) => `theme_translations.#{@theme_id}.${key}`; JS end - - def transpile(es6_source, version) - transpiler = DiscourseJsProcessor::Transpiler.new(skip_module: true) - wrapped = <<~PLUGIN_API_JS - (function() { - if ('Discourse' in window && typeof Discourse._registerPluginCode === 'function') { - #{theme_variables} - Discourse._registerPluginCode('#{version}', api => { - try { - #{es6_source} - } catch(err) { - const rescue = require("discourse/lib/utilities").rescueThemeError; - rescue(__theme_name__, err, api); - } - }); - } - })(); - PLUGIN_API_JS - - transpiler.perform(wrapped) - rescue MiniRacer::RuntimeError => ex - raise CompileError.new ex.message - end end diff --git a/spec/lib/theme_javascript_compiler_spec.rb b/spec/lib/theme_javascript_compiler_spec.rb index e82bb645977..e15adc5ee05 100644 --- a/spec/lib/theme_javascript_compiler_spec.rb +++ b/spec/lib/theme_javascript_compiler_spec.rb @@ -127,4 +127,18 @@ describe ThemeJavascriptCompiler do expect(compiler.content.to_s).to include("addRawTemplate(\"#{name}.hbs\"") end end + + describe "#append_ember_template" do + let(:compiler) { ThemeJavascriptCompiler.new(1, 'marks') } + it 'prepends `javascripts/` to template name if it is not prepended' do + compiler.append_ember_template("/connectors/blah-1", "{{var}}") + expect(compiler.content.to_s).to include('Ember.TEMPLATES["javascripts/connectors/blah-1"]') + + compiler.append_ember_template("connectors/blah-2", "{{var}}") + expect(compiler.content.to_s).to include('Ember.TEMPLATES["javascripts/connectors/blah-2"]') + + compiler.append_ember_template("javascripts/connectors/blah-3", "{{var}}") + expect(compiler.content.to_s).to include('Ember.TEMPLATES["javascripts/connectors/blah-3"]') + end + end end diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 9bb82c9d5cb..6511842e4fa 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -40,6 +40,7 @@ describe RemoteTheme do "stylesheets/file.scss" => ".class1{color:red}", "stylesheets/empty.scss" => "", "javascripts/discourse/controllers/test.js.es6" => "console.log('test');", + "test/acceptance/theme-test.js" => "assert.ok(true);", "common/header.html" => "I AM HEADER", "common/random.html" => "I AM SILLY", "common/embedded.scss" => "EMBED", @@ -74,7 +75,7 @@ describe RemoteTheme do expect(@theme.theme_modifier_set.serialize_topic_excerpts).to eq(true) - expect(@theme.theme_fields.length).to eq(10) + expect(@theme.theme_fields.length).to eq(11) mapped = Hash[*@theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten] @@ -88,8 +89,9 @@ describe RemoteTheme do expect(mapped["3-yaml"]).to eq("boolean_setting: true") expect(mapped["4-en"]).to eq("sometranslations") + expect(mapped["7-acceptance/theme-test.js"]).to eq("assert.ok(true);") - expect(mapped.length).to eq(10) + expect(mapped.length).to eq(11) expect(@theme.settings.length).to eq(1) expect(@theme.settings.first.value).to eq(true) diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 20e00e06940..b57a87d98fb 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -192,34 +192,22 @@ HTML unknown_field = theme.set_field(target: :extra_js, name: "discourse/controllers/discovery.blah", value: "this wont work") theme.save! - expected_js = <<~JS - define("discourse/controllers/discovery", ["discourse/lib/ajax"], function (_ajax) { - "use strict"; + js_field.reload + expect(js_field.value_baked).to include("if ('define' in window) {") + expect(js_field.value_baked).to include("define(\"discourse/theme-#{theme.id}/controllers/discovery\"") + expect(js_field.value_baked).to include("console.log('hello from .js.es6');") - var __theme_name__ = "#{theme.name}"; - - var settings = Discourse.__container__.lookup("service:theme-settings").getObjectForTheme(#{theme.id}); - - var themePrefix = function themePrefix(key) { - return "theme_translations.#{theme.id}.".concat(key); - }; - - console.log('hello from .js.es6'); - }); - JS - expect(js_field.reload.value_baked).to eq(expected_js.strip) - - expect(hbs_field.reload.value_baked).to include('Ember.TEMPLATES["discovery"]') + expect(hbs_field.reload.value_baked).to include('Ember.TEMPLATES["javascripts/discovery"]') expect(raw_hbs_field.reload.value_baked).to include('addRawTemplate("discovery"') expect(hbr_field.reload.value_baked).to include('addRawTemplate("other_discovery"') expect(unknown_field.reload.value_baked).to eq("") expect(unknown_field.reload.error).to eq(I18n.t("themes.compile_error.unrecognized_extension", extension: "blah")) # All together - expect(theme.javascript_cache.content).to include('Ember.TEMPLATES["discovery"]') + expect(theme.javascript_cache.content).to include('Ember.TEMPLATES["javascripts/discovery"]') expect(theme.javascript_cache.content).to include('addRawTemplate("discovery"') - expect(theme.javascript_cache.content).to include('define("discourse/controllers/discovery"') - expect(theme.javascript_cache.content).to include('define("discourse/controllers/discovery-2"') + expect(theme.javascript_cache.content).to include("define(\"discourse/theme-#{theme.id}/controllers/discovery\"") + expect(theme.javascript_cache.content).to include("define(\"discourse/theme-#{theme.id}/controllers/discovery-2\"") expect(theme.javascript_cache.content).to include("var settings =") end diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index ef359c139a3..997bedb4be3 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -126,24 +126,6 @@ describe Theme do expect(Theme.lookup_field(theme.id, :desktop, "head_tag")).to eq("I am bold") end - it "changing theme name should re-transpile HTML theme fields" do - theme.update!(name: "old_name") - html = <<~HTML - - HTML - theme.set_field(target: :common, name: "head_tag", value: html) - theme.save! - field = theme.theme_fields.where(value: html).first - old_value = field.value_baked - - theme.update!(name: "new_name") - field.reload - new_value = field.value_baked - expect(old_value).not_to eq(new_value) - end - it 'should precompile fragments in body and head tags' do with_template = < @@ -276,7 +258,7 @@ HTML def transpile(html) f = ThemeField.create!(target_id: Theme.targets[:mobile], theme_id: 1, name: "after_header", value: html) f.ensure_baked! - [f.value_baked, f.javascript_cache] + [f.value_baked, f.javascript_cache, f] end it "transpiles ES6 code" do @@ -286,10 +268,20 @@ HTML HTML - baked, javascript_cache = transpile(html) + baked, javascript_cache, field = transpile(html) expect(baked).to include(javascript_cache.url) - expect(javascript_cache.content).to include('var x = 1;') - expect(javascript_cache.content).to include("_registerPluginCode('0.1'") + + expect(javascript_cache.content).to include("if ('define' in window) {") + expect(javascript_cache.content).to include( + "define(\"discourse/theme-#{field.theme_id}/initializers/theme-field-#{field.id}-mobile-html-script-1\"" + ) + expect(javascript_cache.content).to include( + "settings = require(\"discourse/lib/theme-settings-store\").getObjectForTheme(#{field.theme_id});" + ) + expect(javascript_cache.content).to include("name: \"theme-field-#{field.id}-mobile-html-script-1\",") + expect(javascript_cache.content).to include("after: \"inject-objects\",") + expect(javascript_cache.content).to include("(0, _pluginApi.withPluginApi)(\"0.1\", function (api) {") + expect(javascript_cache.content).to include("var x = 1;") end it "wraps constants calls in a readOnlyError function" do @@ -369,83 +361,31 @@ HTML theme_field = theme.set_field(target: :common, name: :after_header, value: '') theme.save! - transpiled = <<~HTML - (function() { - if ('Discourse' in window && Discourse.__container__) { - Discourse.__container__ - .lookup("service:theme-settings") - .registerSettings(#{theme.id}, {"name":"bob"}); - } - })(); - (function () { - if ('Discourse' in window && typeof Discourse._registerPluginCode === 'function') { - var __theme_name__ = "awesome theme\\\""; - - var settings = Discourse.__container__.lookup("service:theme-settings").getObjectForTheme(#{theme.id}); - - var themePrefix = function themePrefix(key) { - return "theme_translations.#{theme.id}.".concat(key); - }; - - Discourse._registerPluginCode('1.0', function (api) { - try { - alert(settings.name); - - var a = function a() {}; - } catch (err) { - var rescue = require("discourse/lib/utilities").rescueThemeError; - - rescue(__theme_name__, err, api); - } - }); - } - })(); - HTML - theme_field.reload expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to include(theme_field.javascript_cache.url) - expect(theme_field.javascript_cache.content).to eq(transpiled.strip) + expect(theme_field.javascript_cache.content).to include("if ('require' in window) {") + expect(theme_field.javascript_cache.content).to include( + "require(\"discourse/lib/theme-settings-store\").registerSettings(#{theme_field.theme.id}, {\"name\":\"bob\"});" + ) + expect(theme_field.javascript_cache.content).to include("if ('define' in window) {") + expect(theme_field.javascript_cache.content).to include( + "define(\"discourse/theme-#{theme_field.theme.id}/initializers/theme-field-#{theme_field.id}-common-html-script-1\"," + ) + expect(theme_field.javascript_cache.content).to include("name: \"theme-field-#{theme_field.id}-common-html-script-1\",") + expect(theme_field.javascript_cache.content).to include("after: \"inject-objects\",") + expect(theme_field.javascript_cache.content).to include("(0, _pluginApi.withPluginApi)(\"1.0\", function (api)") + expect(theme_field.javascript_cache.content).to include("alert(settings.name)") + expect(theme_field.javascript_cache.content).to include("var a = function a() {}") setting = theme.settings.find { |s| s.name == :name } setting.value = 'bill' theme.save! - transpiled = <<~HTML - (function() { - if ('Discourse' in window && Discourse.__container__) { - Discourse.__container__ - .lookup("service:theme-settings") - .registerSettings(#{theme.id}, {"name":"bill"}); - } - })(); - (function () { - if ('Discourse' in window && typeof Discourse._registerPluginCode === 'function') { - var __theme_name__ = "awesome theme\\\""; - - var settings = Discourse.__container__.lookup("service:theme-settings").getObjectForTheme(#{theme.id}); - - var themePrefix = function themePrefix(key) { - return "theme_translations.#{theme.id}.".concat(key); - }; - - Discourse._registerPluginCode('1.0', function (api) { - try { - alert(settings.name); - - var a = function a() {}; - } catch (err) { - var rescue = require("discourse/lib/utilities").rescueThemeError; - - rescue(__theme_name__, err, api); - } - }); - } - })(); - HTML - theme_field.reload + expect(theme_field.javascript_cache.content).to include( + "require(\"discourse/lib/theme-settings-store\").registerSettings(#{theme_field.theme.id}, {\"name\":\"bill\"});" + ) expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to include(theme_field.javascript_cache.url) - expect(theme_field.javascript_cache.content).to eq(transpiled.strip) end it 'is empty when the settings are invalid' do diff --git a/spec/requests/qunit_controller_spec.rb b/spec/requests/qunit_controller_spec.rb new file mode 100644 index 00000000000..b0a9535dc2c --- /dev/null +++ b/spec/requests/qunit_controller_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe QunitController do + let(:theme) { Fabricate(:theme, name: 'main-theme') } + let(:component) { Fabricate(:theme, component: true, name: 'enabled-component') } + let(:disabled_component) { Fabricate(:theme, component: true, enabled: false, name: 'disabled-component') } + + before do + Theme.destroy_all + theme.set_default! + component.add_relative_theme!(:parent, theme) + disabled_component.add_relative_theme!(:parent, theme) + [theme, component, disabled_component].each do |t| + t.set_field( + target: :extra_js, + type: :js, + name: "discourse/initializers/my-#{t.id}-initializer.js", + value: "console.log(#{t.id});" + ) + t.set_field( + target: :tests_js, + type: :js, + name: "acceptance/some-test-#{t.id}.js", + value: "assert.ok(#{t.id});" + ) + t.save! + end + end + + context "when no theme is specified" do + it "includes tests of enabled theme + components" do + get '/qunit' + js_urls = JavascriptCache.where(theme_id: [theme.id, component.id]).map(&:url) + expect(js_urls.size).to eq(2) + js_urls.each do |url| + expect(response.body).to include(url) + end + [theme, component].each do |t| + expect(response.body).to include("/theme-javascripts/tests/#{t.id}.js") + end + + js_urls = JavascriptCache.where(theme_id: disabled_component).map(&:url) + expect(js_urls.size).to eq(1) + js_urls.each do |url| + expect(response.body).not_to include(url) + end + expect(response.body).not_to include("/theme-javascripts/tests/#{disabled_component.id}.js") + end + end + + context "when a theme is specified" do + it "includes tests of the specified theme only" do + [theme, disabled_component].each do |t| + get "/qunit?theme_name=#{t.name}" + js_urls = JavascriptCache.where(theme_id: t.id).map(&:url) + expect(js_urls.size).to eq(1) + js_urls.each do |url| + expect(response.body).to include(url) + end + expect(response.body).to include("/theme-javascripts/tests/#{t.id}.js") + + excluded = Theme.pluck(:id) - [t.id] + js_urls = JavascriptCache.where(theme_id: excluded).map(&:url) + expect(js_urls.size).to eq(2) + js_urls.each do |url| + expect(response.body).not_to include(url) + end + excluded.each do |id| + expect(response.body).not_to include("/theme-javascripts/tests/#{id}.js") + end + end + end + end +end diff --git a/spec/requests/theme_javascripts_controller_spec.rb b/spec/requests/theme_javascripts_controller_spec.rb index 6c05efd4e57..b63d9efbbd2 100644 --- a/spec/requests/theme_javascripts_controller_spec.rb +++ b/spec/requests/theme_javascripts_controller_spec.rb @@ -54,4 +54,24 @@ describe ThemeJavascriptsController do end end end + + describe "#show_tests" do + context "theme settings" do + let(:component) { Fabricate(:theme, component: true, name: 'enabled-component') } + + it "forces default values" do + ThemeField.create!( + theme: component, + target_id: Theme.targets[:settings], + name: "yaml", + value: "num_setting: 5" + ) + component.reload + component.update_setting(:num_setting, 643) + + get "/theme-javascripts/tests/#{component.id}.js" + expect(response.body).to include("require(\"discourse/lib/theme-settings-store\").registerSettings(#{component.id}, {\"num_setting\":5}, { force: true });") + end + end + end end