From e060a4f811488dfc9367e264db8e06ae80bbbca3 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 13 Mar 2017 16:30:35 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20=F0=9F=90=9B=20Improve=20theme?= =?UTF-8?q?=20lib,=20middleware=20&=20error=20handling=20(#8145)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue 🎨 simplify loader - use loadOneTheme for init - use loadOneTheme for init - move updateThemeList to the one place that it is used - this just reduces the surface area of the loader 🎨 Move init up to index temporarily - need to figure out what stuff goes in here as well as loading themes - will move it again later once I've got it figured out 🎨 Reorder & cleanup theme middleware - move the order in blog/app.js so that theme middleware isn't called for shared assets - add comments & cleanup in the middleware itself, for clarity 🎨 Simplify the logic in themes middleware - Separate out config dependent on settings changing and config dependent on request - Move blogApp.set('views') - no reason why this isn't in the theme activation method as it's actually simpler if it is there, we already know the active theme exists & can remove the if-guard 🎨 Improve error handling for missing theme - ensure we display a warning - don't have complex logic for handling errors - move loading of an empty hbs object into the error-handler as this will support more cases 🐛 Fix assetHash clearing bug on theme switch - asset hash wasn't correctly being set on theme switch 🎨 Remove themes.read & test loader instead - Previously, we've simplified loader & improved error handling - We are now able to completely remove theme.read as it's nothing more than a wrapper for package.read - This also means we can change our tests from testing the theme reader to loader --- core/server/blog/app.js | 15 +- core/server/middleware/error-handler.js | 6 + core/server/middleware/theme-handler.js | 99 ++++--- core/server/themes/index.js | 28 +- core/server/themes/loader.js | 36 +-- core/server/themes/read.js | 32 --- .../unit/middleware/theme-handler_spec.js | 34 +-- core/test/unit/themes_spec.js | 244 +++++++++--------- 8 files changed, 226 insertions(+), 268 deletions(-) delete mode 100644 core/server/themes/read.js diff --git a/core/server/blog/app.js b/core/server/blog/app.js index f2b523eae4..f02d8d76b4 100644 --- a/core/server/blog/app.js +++ b/core/server/blog/app.js @@ -33,13 +33,6 @@ module.exports = function setupBlogApp() { // set the view engine blogApp.set('view engine', 'hbs'); - // Theme middleware - // rightly or wrongly currently comes before theme static assets - // @TODO revisit where and when these are needed - blogApp.use(themeHandler.updateActiveTheme); - blogApp.use(themeHandler.configHbsForContext); - debug('Themes done'); - // you can extend Ghost with a custom redirects file // see https://github.com/TryGhost/Ghost/issues/7707 customRedirects(blogApp); @@ -58,6 +51,14 @@ module.exports = function setupBlogApp() { // Serve blog images using the storage adapter blogApp.use('/' + utils.url.STATIC_IMAGE_URL_PREFIX, storage.getStorage().serve()); + // Theme middleware + // This should happen AFTER any shared assets are served, as it only changes things to do with templates + // At this point the active theme object is already updated, so we have the right path, so it can probably + // go after staticTheme() as well, however I would really like to simplify this and be certain + blogApp.use(themeHandler.updateActiveTheme); + blogApp.use(themeHandler.configHbsForContext); + debug('Themes done'); + // Theme static assets/files blogApp.use(staticTheme()); debug('Static content done'); diff --git a/core/server/middleware/error-handler.js b/core/server/middleware/error-handler.js index 8d1bf44d51..efba728ff3 100644 --- a/core/server/middleware/error-handler.js +++ b/core/server/middleware/error-handler.js @@ -110,6 +110,12 @@ _private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, /*jshint un templateData.stack = err.stack; } + // It can be that something went wrong with the theme or otherwise loading handlebars + // This ensures that no matter what res.render will work here + if (_.isEmpty(req.app.engines)) { + req.app.engine('hbs', hbs.express3()); + } + res.render(defaultTemplate, templateData, function renderResponse(err, html) { if (!err) { return res.send(html); diff --git a/core/server/middleware/theme-handler.js b/core/server/middleware/theme-handler.js index 65e20f06ce..0b296b0c35 100644 --- a/core/server/middleware/theme-handler.js +++ b/core/server/middleware/theme-handler.js @@ -4,7 +4,6 @@ var _ = require('lodash'), hbs = require('express-hbs'), config = require('../config'), utils = require('../utils'), - logging = require('../logging'), errors = require('../errors'), i18n = require('../i18n'), settingsCache = require('../settings/cache'), @@ -15,10 +14,11 @@ themeHandler = { // ### configHbsForContext Middleware // Setup handlebars for the current context (admin or theme) configHbsForContext: function configHbsForContext(req, res, next) { + // Static information, same for every request unless the settings change + // @TODO: bind this once and then update based on events? var themeData = { title: settingsCache.get('title'), description: settingsCache.get('description'), - url: utils.url.urlFor('home', {secure: req.secure}, true), facebook: settingsCache.get('facebook'), twitter: settingsCache.get('twitter'), timezone: settingsCache.get('activeTimezone'), @@ -29,9 +29,17 @@ themeHandler = { logo: settingsCache.get('logo'), amp: settingsCache.get('amp') }, - labsData = _.cloneDeep(settingsCache.get('labs')), - blogApp = req.app; + labsData = _.cloneDeep(settingsCache.get('labs')); + // Request-specific information + // These things are super dependent on the request, so they need to be in middleware + themeData.url = utils.url.urlFor('home', {secure: req.secure}, true); + + // Pass 'secure' flag to the view engine + // so that templates can choose to render https or http 'url', see url utility + res.locals.secure = req.secure; + + // @TODO: only do this if something changed? hbs.updateTemplateOptions({ data: { blog: themeData, @@ -39,47 +47,38 @@ themeHandler = { } }); - if (config.getContentPath('themes') && blogApp.get('activeTheme')) { - blogApp.set('views', path.join(config.getContentPath('themes'), blogApp.get('activeTheme'))); - } - - // Pass 'secure' flag to the view engine - // so that templates can choose to render https or http 'url', see url utility - res.locals.secure = req.secure; - next(); }, // ### Activate Theme // Helper for updateActiveTheme - activateTheme: function activateTheme(blogApp, activeTheme) { - var hbsOptions, - themePartials = path.join(config.getContentPath('themes'), activeTheme, 'partials'); + activateTheme: function activateTheme(blogApp, activeThemeName) { + var themePartialsPath = path.join(config.getContentPath('themes'), activeThemeName, 'partials'), + hbsOptions = { + partialsDir: [config.get('paths').helperTemplates], + onCompile: function onCompile(exhbs, source) { + return exhbs.handlebars.compile(source, {preventIndent: true}); + } + }; - // clear the view cache - blogApp.cache = {}; - // reset the asset hash - config.assetHash = null; - - // set view engine - hbsOptions = { - partialsDir: [config.get('paths').helperTemplates], - onCompile: function onCompile(exhbs, source) { - return exhbs.handlebars.compile(source, {preventIndent: true}); - } - }; - - fs.stat(themePartials, function stat(err, stats) { + fs.stat(themePartialsPath, function stat(err, stats) { // Check that the theme has a partials directory before trying to use it if (!err && stats && stats.isDirectory()) { - hbsOptions.partialsDir.push(themePartials); + hbsOptions.partialsDir.push(themePartialsPath); } }); + // reset the asset hash + config.set('assetHash', null); + // clear the view cache + blogApp.cache = {}; + // Set the views and engine + blogApp.set('views', path.join(config.getContentPath('themes'), activeThemeName)); blogApp.engine('hbs', hbs.express3(hbsOptions)); // Set active theme variable on the express server - blogApp.set('activeTheme', activeTheme); + // Note: this is effectively the "mounted" theme, which has been loaded into the express app + blogApp.set('activeTheme', activeThemeName); }, // ### updateActiveTheme @@ -88,31 +87,21 @@ themeHandler = { // is not yet activated. updateActiveTheme: function updateActiveTheme(req, res, next) { var blogApp = req.app, - activeTheme = settingsCache.get('activeTheme'); + activeThemeName = settingsCache.get('activeTheme'), + mountedThemeName = blogApp.get('activeTheme'); - // Check if the theme changed - if (activeTheme !== blogApp.get('activeTheme')) { - // Change theme - if (!themeList.get(activeTheme)) { - if (!res.isAdmin) { - // Trying to start up without the active theme present, setup a simple hbs instance - // and render an error page straight away. - blogApp.engine('hbs', hbs.express3()); - return next(new errors.NotFoundError({ - message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme}) - })); - } else { - // At this point the activated theme is not present and the current - // request is for the admin client. In order to allow the user access - // to the admin client we set an hbs instance on the app so that middleware - // processing can continue. - blogApp.engine('hbs', hbs.express3()); - logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme})); - return next(); - } - } else { - themeHandler.activateTheme(blogApp, activeTheme); - } + // This means that the theme hasn't been loaded yet i.e. there is no active theme + if (!themeList.get(activeThemeName)) { + // This is the one place we ACTUALLY throw an error for a missing theme + // As it's a request we cannot serve + return next(new errors.InternalServerError({ + message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName}) + })); + + // If there is an active theme AND it has changed, call activate + } else if (activeThemeName !== mountedThemeName) { + // This is effectively "mounting" a theme into express, the theme is already "active" + themeHandler.activateTheme(blogApp, activeThemeName); } next(); diff --git a/core/server/themes/index.js b/core/server/themes/index.js index 4de338d77d..2ff98497fa 100644 --- a/core/server/themes/index.js +++ b/core/server/themes/index.js @@ -1,9 +1,33 @@ -var themeLoader = require('./loader'); +var debug = require('debug')('ghost:themes'), + events = require('../events'), + logging = require('../logging'), + i18n = require('../i18n'), + themeLoader = require('./loader'), + settingsCache = require('../settings/cache'); // @TODO: reduce the amount of things we expose to the outside world // Make this a nice clean sensible API we can all understand! module.exports = { - init: themeLoader.init, + // Init themes module + // TODO: move this once we're clear what needs to happen here + init: function initThemes() { + var activeThemeName = settingsCache.get('activeTheme'); + debug('init themes', activeThemeName); + + // Register a listener for server-start to load all themes + events.on('server:start', function readAllThemesOnServerStart() { + themeLoader.loadAllThemes(); + }); + + // Just read the active theme for now + return themeLoader + .loadOneTheme(activeThemeName) + .catch(function () { + // Active theme is missing, we don't want to exit because the admin panel will still work + logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName})); + }); + }, + // Load themes, soon to be removed and exposed via specific function. loadAll: themeLoader.loadAllThemes, loadOne: themeLoader.loadOneTheme, list: require('./list'), diff --git a/core/server/themes/loader.js b/core/server/themes/loader.js index e36cadce11..6d80aebfb4 100644 --- a/core/server/themes/loader.js +++ b/core/server/themes/loader.js @@ -1,50 +1,30 @@ var debug = require('debug')('ghost:themes:loader'), config = require('../config'), - events = require('../events'), themeList = require('./list'), - read = require('./read'), - settingsCache = require('../settings/cache'), - updateThemeList, + read = require('../utils/packages').read, loadAllThemes, - loadOneTheme, - initThemes; - -updateThemeList = function updateThemeList(themes) { - debug('loading themes', Object.keys(themes)); - themeList.init(themes); -}; + loadOneTheme; loadAllThemes = function loadAllThemes() { return read .all(config.getContentPath('themes')) - .then(updateThemeList); + .then(function updateThemeList(themes) { + debug('loading themes', Object.keys(themes)); + + themeList.init(themes); + }); }; loadOneTheme = function loadOneTheme(themeName) { return read .one(config.getContentPath('themes'), themeName) .then(function (readThemes) { - // @TODO change read one to not return a keyed object + debug('loaded one theme', themeName); return themeList.set(themeName, readThemes[themeName]); }); }; -initThemes = function initThemes() { - debug('init themes', settingsCache.get('activeTheme')); - - // Register a listener for server-start to load all themes - events.on('server:start', function readAllThemesOnServerStart() { - loadAllThemes(); - }); - - // Just read the active theme for now - return read - .one(config.getContentPath('themes'), settingsCache.get('activeTheme')) - .then(updateThemeList); -}; - module.exports = { - init: initThemes, loadAllThemes: loadAllThemes, loadOneTheme: loadOneTheme }; diff --git a/core/server/themes/read.js b/core/server/themes/read.js deleted file mode 100644 index d024502766..0000000000 --- a/core/server/themes/read.js +++ /dev/null @@ -1,32 +0,0 @@ -/** - * # Read Themes - * - * Util that wraps packages.read - */ -var packages = require('../utils/packages'), - logging = require('../logging'), - i18n = require('../i18n'), - - readOneTheme, - readAllThemes; - -readOneTheme = function readOneTheme(dir, name) { - return packages - .read.one(dir, name) - .catch(function () { - // For now we return an empty object as this is not fatal unless the frontend of the blog is requested - logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: name})); - return {}; - }); -}; - -readAllThemes = function readAllThemes(dir) { - return packages.read.all(dir); -}; - -/** - * Expose public API - */ - -module.exports.all = readAllThemes; -module.exports.one = readOneTheme; diff --git a/core/test/unit/middleware/theme-handler_spec.js b/core/test/unit/middleware/theme-handler_spec.js index 33c47df4f3..92d8b1ef51 100644 --- a/core/test/unit/middleware/theme-handler_spec.js +++ b/core/test/unit/middleware/theme-handler_spec.js @@ -1,14 +1,13 @@ -var sinon = require('sinon'), - should = require('should'), - express = require('express'), - fs = require('fs'), - hbs = require('express-hbs'), - themeList = require('../../../server/themes').list, +var sinon = require('sinon'), + should = require('should'), + express = require('express'), + fs = require('fs'), + hbs = require('express-hbs'), + themeList = require('../../../server/themes').list, themeHandler = require('../../../server/middleware/theme-handler'), - logging = require('../../../server/logging'), - settingsCache = require('../../../server/settings/cache'), + settingsCache = require('../../../server/settings/cache'), - sandbox = sinon.sandbox.create(); + sandbox = sinon.sandbox.create(); describe('Theme Handler', function () { var req, res, next, blogApp; @@ -130,22 +129,5 @@ describe('Theme Handler', function () { done(); }); }); - - it('throws only warns if theme is missing for admin req', function (done) { - var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'), - loggingWarnStub = sandbox.spy(logging, 'warn'); - - sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('rasper'); - - res.isAdmin = true; - blogApp.set('activeTheme', 'not-casper'); - - themeHandler.updateActiveTheme(req, res, function () { - activateThemeSpy.called.should.be.false(); - loggingWarnStub.called.should.be.true(); - loggingWarnStub.calledWith('The currently active theme "rasper" is missing.').should.be.true(); - done(); - }); - }); }); }); diff --git a/core/test/unit/themes_spec.js b/core/test/unit/themes_spec.js index e0caefa785..f6996205ee 100644 --- a/core/test/unit/themes_spec.js +++ b/core/test/unit/themes_spec.js @@ -1,11 +1,12 @@ -var should = require('should'), - sinon = require('sinon'), - _ = require('lodash'), - fs = require('fs'), - tmp = require('tmp'), - join = require('path').join, - themeList = require('../../server/themes').list, - readThemes = require('../../server/themes/read'), +var should = require('should'), + sinon = require('sinon'), + _ = require('lodash'), + fs = require('fs'), + tmp = require('tmp'), + join = require('path').join, + config = require('../../server/config'), + themes = require('../../server/themes'), + themeList = themes.list, sandbox = sinon.sandbox.create(); @@ -13,126 +14,133 @@ var should = require('should'), should.equal(true, true); describe('Themes', function () { - afterEach(function () { - sandbox.restore(); - }); + describe('Loader', function () { + var themePath; - describe('Read All', function () { - it('should read directory and include only folders', function (done) { - var themePath = tmp.dirSync({unsafeCleanup: true}); - - // create trash - fs.writeFileSync(join(themePath.name, 'casper.zip')); - fs.writeFileSync(join(themePath.name, '.DS_Store')); - - // create actual theme - fs.mkdirSync(join(themePath.name, 'casper')); - fs.mkdirSync(join(themePath.name, 'casper', 'partials')); - fs.writeFileSync(join(themePath.name, 'casper', 'index.hbs')); - fs.writeFileSync(join(themePath.name, 'casper', 'partials', 'navigation.hbs')); - - readThemes.all(themePath.name) - .then(function (themeList) { - themeList.should.eql({ - casper: { - name: 'casper', - path: join(themePath.name, 'casper'), - 'package.json': null - } - }); - - done(); - }) - .catch(done) - .finally(themePath.removeCallback); + beforeEach(function () { + themePath = tmp.dirSync({unsafeCleanup: true}); + sandbox.stub(config, 'getContentPath').withArgs('themes').returns(themePath.name); }); - it('should read directory and read package.json if present', function (done) { - var themePath = tmp.dirSync({unsafeCleanup: true}); + afterEach(function () { + themePath.removeCallback(); + sandbox.restore(); + }); - // create trash - fs.writeFileSync(join(themePath.name, 'README.md')); - fs.writeFileSync(join(themePath.name, 'Thumbs.db')); + describe('Load All', function () { + it('should load directory and include only folders', function (done) { + // create trash + fs.writeFileSync(join(themePath.name, 'casper.zip')); + fs.writeFileSync(join(themePath.name, '.DS_Store')); - // create actual theme - fs.mkdirSync(join(themePath.name, 'casper')); - fs.mkdirSync(join(themePath.name, 'not-casper')); - fs.writeFileSync( - join(themePath.name, 'casper', 'package.json'), - JSON.stringify({name: 'casper', version: '0.1.2'}) - ); + // create actual theme + fs.mkdirSync(join(themePath.name, 'casper')); + fs.mkdirSync(join(themePath.name, 'casper', 'partials')); + fs.writeFileSync(join(themePath.name, 'casper', 'index.hbs')); + fs.writeFileSync(join(themePath.name, 'casper', 'partials', 'navigation.hbs')); - readThemes.all(themePath.name) - .then(function (themeList) { - themeList.should.eql({ - casper: { + themes.loadAll() + .then(function (result) { + var themeResult = themeList.getAll(); + + // Loader doesn't return anything + should.not.exist(result); + + themeResult.should.eql({ + casper: { + name: 'casper', + path: join(themePath.name, 'casper'), + 'package.json': null + } + }); + + done(); + }) + .catch(done); + }); + + it('should read directory and read package.json if present', function (done) { + // create trash + fs.writeFileSync(join(themePath.name, 'README.md')); + fs.writeFileSync(join(themePath.name, 'Thumbs.db')); + + // create actual theme + fs.mkdirSync(join(themePath.name, 'casper')); + fs.mkdirSync(join(themePath.name, 'not-casper')); + fs.writeFileSync( + join(themePath.name, 'casper', 'package.json'), + JSON.stringify({name: 'casper', version: '0.1.2'}) + ); + + themes.loadAll() + .then(function (result) { + var themeResult = themeList.getAll(); + + // Loader doesn't return anything + should.not.exist(result); + + themeResult.should.eql({ + casper: { + name: 'casper', + path: join(themePath.name, 'casper'), + 'package.json': {name: 'casper', version: '0.1.2'} + }, + 'not-casper': { + name: 'not-casper', + path: join(themePath.name, 'not-casper'), + 'package.json': null + } + }); + + done(); + }) + .catch(done); + }); + }); + + describe('Load One', function () { + it('should read directory and include only single requested theme', function (done) { + // create trash + fs.writeFileSync(join(themePath.name, 'casper.zip')); + fs.writeFileSync(join(themePath.name, '.DS_Store')); + + // create actual theme + fs.mkdirSync(join(themePath.name, 'casper')); + fs.writeFileSync(join(themePath.name, 'casper', 'index.hbs')); + fs.writeFileSync( + join(themePath.name, 'casper', 'package.json'), + JSON.stringify({name: 'casper', version: '0.1.2'}) + ); + fs.mkdirSync(join(themePath.name, 'not-casper')); + fs.writeFileSync(join(themePath.name, 'not-casper', 'index.hbs')); + + themes.loadOne('casper') + .then(function (themeResult) { + themeResult.should.eql({ name: 'casper', path: join(themePath.name, 'casper'), 'package.json': {name: 'casper', version: '0.1.2'} - }, - 'not-casper': { - name: 'not-casper', - path: join(themePath.name, 'not-casper'), - 'package.json': null - } + }); + + done(); + }) + .catch(done); + }); + + it('should throw an error if theme cannot be found', function (done) { + // create trash + fs.writeFileSync(join(themePath.name, 'casper.zip')); + fs.writeFileSync(join(themePath.name, '.DS_Store')); + + themes.loadOne('casper') + .then(function () { + done('Should have thrown an error'); + }) + .catch(function (err) { + err.message.should.eql('Package not found'); + done(); }); - - done(); - }) - .catch(done) - .finally(themePath.removeCallback); - }); - }); - - describe('Read One', function () { - it('should read directory and include only single requested theme', function (done) { - var themePath = tmp.dirSync({unsafeCleanup: true}); - - // create trash - fs.writeFileSync(join(themePath.name, 'casper.zip')); - fs.writeFileSync(join(themePath.name, '.DS_Store')); - - // create actual theme - fs.mkdirSync(join(themePath.name, 'casper')); - fs.writeFileSync(join(themePath.name, 'casper', 'index.hbs')); - fs.writeFileSync( - join(themePath.name, 'casper', 'package.json'), - JSON.stringify({name: 'casper', version: '0.1.2'}) - ); - fs.mkdirSync(join(themePath.name, 'not-casper')); - fs.writeFileSync(join(themePath.name, 'not-casper', 'index.hbs')); - - readThemes.one(themePath.name, 'casper') - .then(function (themeList) { - themeList.should.eql({ - casper: { - name: 'casper', - path: join(themePath.name, 'casper'), - 'package.json': {name: 'casper', version: '0.1.2'} - } - }); - - done(); - }) - .catch(done) - .finally(themePath.removeCallback); - }); - - it('should return empty object if theme cannot be found', function (done) { - var themePath = tmp.dirSync({unsafeCleanup: true}); - - // create trash - fs.writeFileSync(join(themePath.name, 'casper.zip')); - fs.writeFileSync(join(themePath.name, '.DS_Store')); - - readThemes.one(themePath.name, 'casper') - .then(function (themeList) { - themeList.should.eql({}); - - done(); - }) - .catch(done) - .finally(themePath.removeCallback); + }); }); });