From af09d55c7a18cccbc8de0151f94614d1550cf092 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 27 Apr 2021 12:49:48 +0100 Subject: [PATCH] Moved activate from themes to the bridge refs: https://github.com/TryGhost/Ghost/commit/bf0823c9a2ddbc93ad0ddcec36eed130c8c5a203 - continuing the work of splitting up the theme service into logical components Theme activations are a trickier piece of the theme split puzzle because they are called from the API and theme service on boot in different ways. Activations require a theme to have been validated at different levels. Validations are also tricky - do they belong to the theme engine, or the theme service? There are then several different flows for activations: - On Boot - API "activate" call - API override on upload or install via setFromZip, which is a method in the storage layer These calls all have quite different logical flows at the moment, and need to be unified For now, I've moved the existing "activate" function onto the bridge. This allows the theme service to be split from the frontend, and refactoring can start from there. I hope to move this so there is less code in the actual bridge very soon, but my goal is not to require any server packages in the frontend part of this I think ideally: - all activation code, including validation, should probably be part of the theme engine - the theme engine should offer 3 methods: getActive() canActivate() and activate() - the theme service is then only responsible for loading themes in and out of storage, JSON responses for the API, and handing themes to the frontend via the bridge at the appropriate moment --- core/bridge.js | 41 +++++++++++++++++++- core/frontend/services/theme-engine/index.js | 5 ++- core/frontend/services/themes/activate.js | 32 --------------- core/frontend/services/themes/index.js | 8 ++-- core/frontend/services/themes/storage.js | 4 +- 5 files changed, 49 insertions(+), 41 deletions(-) delete mode 100644 core/frontend/services/themes/activate.js diff --git a/core/bridge.js b/core/bridge.js index d807d4da9d..02ea4c54ed 100644 --- a/core/bridge.js +++ b/core/bridge.js @@ -1,6 +1,17 @@ -// @TODO: refactor constructor pattern so we don't have to require config here? +/** + * The Bridge + * + * The bridge is responsible for handing communication from the server to the frontend. + * Data should only be flowing server -> frontend. + * As the architecture improves, the number of cross requires here should go down + * Eventually, the aim is to make this a component that is initialised on boot and is either handed to or actively creates the frontend, if the frontend is desired. + * + * This file is a great place for all the cross-component event handling in lieu of refactoring + */ +const errors = require('@tryghost/errors'); const config = require('./shared/config'); -const {events} = require('./server/lib/common'); +const logging = require('./shared/logging'); +const {events, i18n} = require('./server/lib/common'); const themeEngine = require('./frontend/services/theme-engine'); class Bridge { @@ -18,6 +29,32 @@ class Bridge { return themeEngine.getActive(); } + activateTheme(loadedTheme, checkedTheme, error) { + // no need to check the score, activation should be used in combination with validate.check + // Use the two theme objects to set the current active theme + try { + let previousGhostAPI; + + if (this.getActiveTheme()) { + previousGhostAPI = this.getActiveTheme().engine('ghost-api'); + } + + themeEngine.setActive(loadedTheme, checkedTheme, error); + const currentGhostAPI = this.getActiveTheme().engine('ghost-api'); + + if (previousGhostAPI !== undefined && (previousGhostAPI !== currentGhostAPI)) { + events.emit('services.themes.api.changed'); + const siteApp = require('./server/web/site/app'); + siteApp.reload(); + } + } catch (err) { + logging.error(new errors.InternalServerError({ + message: i18n.t('errors.middleware.themehandler.activateFailed', {theme: loadedTheme.name}), + err: err + })); + } + } + getFrontendApiVersion() { if (this.getActiveTheme()) { return this.getActiveTheme().engine('ghost-api'); diff --git a/core/frontend/services/theme-engine/index.js b/core/frontend/services/theme-engine/index.js index ef99cd0506..85f6b05bdc 100644 --- a/core/frontend/services/theme-engine/index.js +++ b/core/frontend/services/theme-engine/index.js @@ -1,5 +1,8 @@ +const active = require('./active'); + module.exports = { + getActive: active.get, + setActive: active.set, loadCoreHelpers: require('./handlebars/helpers').loadCoreHelpers, - getActive: require('./active').get, middleware: require('./middleware') }; diff --git a/core/frontend/services/themes/activate.js b/core/frontend/services/themes/activate.js deleted file mode 100644 index 4c5861b9f4..0000000000 --- a/core/frontend/services/themes/activate.js +++ /dev/null @@ -1,32 +0,0 @@ -const {events, i18n} = require('../../../server/lib/common'); -const logging = require('../../../shared/logging'); -const errors = require('@tryghost/errors'); -const active = require('../theme-engine/active'); - -function activate(loadedTheme, checkedTheme, error) { - // no need to check the score, activation should be used in combination with validate.check - // Use the two theme objects to set the current active theme - try { - let previousGhostAPI; - - if (active.get()) { - previousGhostAPI = active.get().engine('ghost-api'); - } - - active.set(loadedTheme, checkedTheme, error); - const currentGhostAPI = active.get().engine('ghost-api'); - - if (previousGhostAPI !== undefined && (previousGhostAPI !== currentGhostAPI)) { - events.emit('services.themes.api.changed'); - const siteApp = require('../../../server/web/site/app'); - siteApp.reload(); - } - } catch (err) { - logging.error(new errors.InternalServerError({ - message: i18n.t('errors.middleware.themehandler.activateFailed', {theme: loadedTheme.name}), - err: err - })); - } -} - -module.exports = activate; diff --git a/core/frontend/services/themes/index.js b/core/frontend/services/themes/index.js index a1c35bd01d..b399d0410e 100644 --- a/core/frontend/services/themes/index.js +++ b/core/frontend/services/themes/index.js @@ -4,7 +4,7 @@ const {i18n: commonI18n} = require('../proxy'); const logging = require('../../../shared/logging'); const errors = require('@tryghost/errors'); const themeLoader = require('./loader'); -const activate = require('./activate'); +const bridge = require('../../../bridge'); const validate = require('./validate'); const list = require('./list'); const settingsCache = require('../../../server/services/settings/cache'); @@ -36,7 +36,7 @@ module.exports = { logging.error(checkError); - activate(theme, checkedTheme, checkError); + bridge.activateTheme(theme, checkedTheme, checkError); } else { // CASE: inform that the theme has errors, but not fatal (theme still works) if (checkedTheme.results.error.length) { @@ -53,7 +53,7 @@ module.exports = { debug('Activating theme (method A on boot)', activeThemeName); - activate(theme, checkedTheme); + bridge.activateTheme(theme, checkedTheme); } }); }) @@ -82,7 +82,7 @@ module.exports = { return validate.checkSafe(loadedTheme) .then((checkedTheme) => { debug('Activating theme (method B on API "activate")', themeName); - activate(loadedTheme, checkedTheme); + bridge.activateTheme(loadedTheme, checkedTheme); return checkedTheme; }); diff --git a/core/frontend/services/themes/storage.js b/core/frontend/services/themes/storage.js index 8429cc727d..a4f889e0bc 100644 --- a/core/frontend/services/themes/storage.js +++ b/core/frontend/services/themes/storage.js @@ -1,6 +1,6 @@ const fs = require('fs-extra'); -const activate = require('./activate'); +const bridge = require('../../../bridge'); const validate = require('./validate'); const list = require('./list'); const ThemeStorage = require('./ThemeStorage'); @@ -79,7 +79,7 @@ module.exports = { // CASE: if this is the active theme, we are overriding if (overrideTheme) { debug('Activating theme (method C, on API "override")', shortName); - activate(loadedTheme, checkedTheme); + bridge.activateTheme(loadedTheme, checkedTheme); } // @TODO: unify the name across gscan and Ghost!