From 8e5b945b0ffd8ab5f5156ae52e460cef0136a551 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 11 Feb 2022 11:30:36 +0800 Subject: [PATCH] Revert "DEV: Improve theme error handling UX" (#15900) `PrettyText.cook` is breaking on some sites. Revert for now while we investigate. This reverts commit c81d369ab69cf1a86d92fa9a93fae13f82902bed. --- app/assets/javascripts/discourse/app/app.js | 5 -- .../discourse/app/lib/source-identifier.js | 46 -------------- .../pre-initializers/theme-errors-handler.js | 63 +++++-------------- .../stylesheets/common/base/discourse.scss | 11 +--- app/models/theme.rb | 4 +- app/models/theme_field.rb | 4 +- config/locales/client.en.yml | 6 +- spec/models/theme_field_spec.rb | 8 +-- 8 files changed, 28 insertions(+), 119 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/lib/source-identifier.js diff --git a/app/assets/javascripts/discourse/app/app.js b/app/assets/javascripts/discourse/app/app.js index 19da3d28362..bce247db3c5 100644 --- a/app/assets/javascripts/discourse/app/app.js +++ b/app/assets/javascripts/discourse/app/app.js @@ -52,11 +52,6 @@ const Discourse = Application.extend({ start() { document.querySelector("noscript")?.remove(); - if (Error.stackTraceLimit) { - // We need Errors to have full stack traces for `lib/source-identifier` - Error.stackTraceLimit = Infinity; - } - Object.keys(requirejs._eak_seen).forEach((key) => { if (/\/pre\-initializers\//.test(key)) { const initializer = this._prepareInitializer(key); diff --git a/app/assets/javascripts/discourse/app/lib/source-identifier.js b/app/assets/javascripts/discourse/app/lib/source-identifier.js deleted file mode 100644 index 0ba32e30a4b..00000000000 --- a/app/assets/javascripts/discourse/app/lib/source-identifier.js +++ /dev/null @@ -1,46 +0,0 @@ -import getURL from "discourse-common/lib/get-url"; -import PreloadStore from "discourse/lib/preload-store"; - -export default function identifySource(error) { - if (!error || !error.stack) { - try { - throw new Error("Source identification error"); - } catch (e) { - error = e; - } - } - - if (!error.stack) { - return; - } - - const themeMatches = - error.stack.match(/\/theme-javascripts\/[\w-]+\.js/g) || []; - - for (const match of themeMatches) { - const scriptElement = document.querySelector(`script[src*="${match}"`); - if (scriptElement?.dataset.themeId) { - return { - type: "theme", - ...getThemeInfo(scriptElement.dataset.themeId), - }; - } - } -} - -export function getThemeInfo(id) { - const name = PreloadStore.get("activatedThemes")[id] || `(theme-id: ${id})`; - return { - id, - name, - path: getURL(`/admin/customize/themes/${id}?safe_mode=no_custom`), - }; -} - -export function consolePrefix(error, source) { - source = source || identifySource(error); - if (source && source.type === "theme") { - return `[THEME ${source.id} '${source.name}']`; - } - return ""; -} 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 index af3f4cfdcc9..98a2f914396 100644 --- a/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js +++ b/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js @@ -1,13 +1,9 @@ import { isTesting } from "discourse-common/config/environment"; import { getAndClearUnhandledThemeErrors } from "discourse/app"; +import PreloadStore from "discourse/lib/preload-store"; import getURL from "discourse-common/lib/get-url"; import I18n from "I18n"; import { bind } from "discourse-common/utils/decorators"; -import { escape } from "pretty-text/sanitizer"; -import identifySource, { - consolePrefix, - getThemeInfo, -} from "discourse/lib/source-identifier"; const showingErrors = new Set(); @@ -60,64 +56,39 @@ function reportToLogster(name, error) { function reportThemeError(currentUser, e) { const { themeId, error } = e.detail; - const source = { - type: "theme", - ...getThemeInfo(themeId), - }; + 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); - reportToConsole(error, source); - reportToLogster(source.name, error); - - const message = I18n.t("themes.broken_theme_alert"); - displayErrorNotice(currentUser, message, source); + const path = getURL("/admin/customize/themes"); + const message = I18n.t("themes.broken_theme_alert", { + theme: name, + path: `${path}`, + }); + displayErrorNotice(currentUser, message); } function reportGenericError(currentUser, e) { const { messageKey, error } = e.detail; - let message = I18n.t(messageKey); - - const source = identifySource(error); - - reportToConsole(error, source); + /* eslint-disable-next-line no-console */ + console.error(error); if (messageKey && !showingErrors.has(messageKey)) { showingErrors.add(messageKey); - displayErrorNotice(currentUser, message, source); + displayErrorNotice(currentUser, I18n.t(messageKey)); } } -function reportToConsole(error, source) { - const prefix = consolePrefix(error, source); - if (prefix) { - /* eslint-disable-next-line no-console */ - console.error(prefix, error); - } else { - /* eslint-disable-next-line no-console */ - console.error(error); - } -} - -function displayErrorNotice(currentUser, message, source) { +function displayErrorNotice(currentUser, message) { if (!currentUser?.admin) { return; } - let html = `⚠️ ${message}`; - - if (source && source.type === "theme") { - html += `
${I18n.t("themes.error_caused_by", { - name: escape(source.name), - path: source.path, - })}`; - } - - html += `
${I18n.t( - "themes.only_admins" - )}`; - const alertDiv = document.createElement("div"); alertDiv.classList.add("broken-theme-alert"); - alertDiv.innerHTML = html; + alertDiv.innerHTML = `⚠️ ${message}`; document.body.prepend(alertDiv); } diff --git a/app/assets/stylesheets/common/base/discourse.scss b/app/assets/stylesheets/common/base/discourse.scss index 0126ae6cc55..9b253d36b8e 100644 --- a/app/assets/stylesheets/common/base/discourse.scss +++ b/app/assets/stylesheets/common/base/discourse.scss @@ -830,19 +830,10 @@ table { font-size: $base-font-size; font-weight: bold; padding: 5px 0; - background: var(--danger); + background: var(--danger-medium); text-align: center; z-index: z("max"); color: var(--secondary); - - a { - color: var(--secondary); - text-decoration: underline; - } - - .theme-error-suffix { - font-weight: normal; - } } .controls { diff --git a/app/models/theme.rb b/app/models/theme.rb index 2fea0ca32f4..c7b3def3050 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -152,7 +152,7 @@ class Theme < ActiveRecord::Base SvgSprite.expire_cache end - BASE_COMPILER_VERSION = 52 + BASE_COMPILER_VERSION = 51 def self.compiler_version get_set_cache "compiler_version" do dependencies = [ @@ -389,7 +389,7 @@ class Theme < ActiveRecord::Base end caches = JavascriptCache.where(theme_id: theme_ids) caches = caches.sort_by { |cache| theme_ids.index(cache.theme_id) } - return caches.map { |c| "" }.join("\n") + return caches.map { |c| "" }.join("\n") end list_baked_fields(theme_ids, target, name).map { |f| f.value_baked || f.value }.join("\n") end diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 162d80b1e48..fac8bf1b781 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -142,7 +142,7 @@ class ThemeField < ActiveRecord::Base javascript_cache.content = js_compiler.content javascript_cache.save! - doc.add_child("") if javascript_cache.content.present? + doc.add_child("") if javascript_cache.content.present? [doc.to_s, errors&.join("\n")] end @@ -258,7 +258,7 @@ class ThemeField < ActiveRecord::Base javascript_cache.content = js_compiler.content javascript_cache.save! doc = "" - doc = "" if javascript_cache.content.present? + doc = "" if javascript_cache.content.present? [doc, errors&.join("\n")] end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a778b2e6763..80b7b8fda1d 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -194,11 +194,9 @@ en: themes: default_description: "Default" - broken_theme_alert: "Your site may not work because a theme / component has errors." - error_caused_by: "Caused by '%{name}'. Click here to reconfigure or disable." - only_admins: "(this message is only shown to site administrators)" + broken_theme_alert: "Your site may not work because theme / component %{theme} has errors. Disable it at %{path}." - broken_decorator_alert: "Posts may not display correctly because one of the post content decorators on your site raised an error." + broken_decorator_alert: "Posts may not display correctly because one of the post content decorators on your site raised an error. Check the browser developer tools for more information." s3: regions: diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index d09c7625b61..63b2cc2b67a 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -77,7 +77,7 @@ describe ThemeField do theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html) theme_field.ensure_baked! - expect(theme_field.value_baked).to include("") + expect(theme_field.value_baked).to include("") expect(theme_field.value_baked).to include("external-script.js") expect(theme_field.value_baked).to include('") + expect(field.value_baked).to include("") expect(field.javascript_cache.content).to include("Theme Transpilation Error:") field.update!(value: '') @@ -132,7 +132,7 @@ HTML theme_field.ensure_baked! javascript_cache = theme_field.javascript_cache - expect(theme_field.value_baked).to include("") + expect(theme_field.value_baked).to include("") expect(javascript_cache.content).to include("testing-div") expect(javascript_cache.content).to include("string_setting") expect(javascript_cache.content).to include("test text \\\" 123!") @@ -380,7 +380,7 @@ HTML describe "javascript cache" do it "is generated correctly" do fr1.ensure_baked! - expect(fr1.value_baked).to include("") + expect(fr1.value_baked).to include("") expect(fr1.javascript_cache.content).to include("bonjourworld") expect(fr1.javascript_cache.content).to include("helloworld") expect(fr1.javascript_cache.content).to include("enval1")