🎨 🐛 Improve theme lib, middleware & error handling (#8145)

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

@@ -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');

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);

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')),
+            labsData = _.cloneDeep(settingsCache.get('labs'));
-            blogApp = req.app;
 
+        // 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) {
+    activateTheme: function activateTheme(blogApp, activeThemeName) {
-        var hbsOptions,
+        var themePartialsPath = path.join(config.getContentPath('themes'), activeThemeName, 'partials'),
-            themePartials = path.join(config.getContentPath('themes'), activeTheme, 'partials');
+            hbsOptions = {
+                partialsDir: [config.get('paths').helperTemplates],
+                onCompile: function onCompile(exhbs, source) {
+                    return exhbs.handlebars.compile(source, {preventIndent: true});
+                }
+            };
 
-        // clear the view cache
+        fs.stat(themePartialsPath, function stat(err, stats) {
-        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) {
             // 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
+        // This means that the theme hasn't been loaded yet i.e. there is no active theme
-        if (activeTheme !== blogApp.get('activeTheme')) {
+        if (!themeList.get(activeThemeName)) {
-            // Change theme
+            // This is the one place we ACTUALLY throw an error for a missing theme
-            if (!themeList.get(activeTheme)) {
+            // As it's a request we cannot serve
-                if (!res.isAdmin) {
+            return next(new errors.InternalServerError({
-                    // Trying to start up without the active theme present, setup a simple hbs instance
+                message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName})
-                    // and render an error page straight away.
+            }));
-                    blogApp.engine('hbs', hbs.express3());
+
-                    return next(new errors.NotFoundError({
+            // If there is an active theme AND it has changed, call activate
-                        message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme})
+        } else if (activeThemeName !== mountedThemeName) {
-                    }));
+            // This is effectively "mounting" a theme into express, the theme is already "active"
-                } else {
+            themeHandler.activateTheme(blogApp, activeThemeName);
-                    // 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);
-            }
         }
 
         next();

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'),

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'),
+    read = require('../utils/packages').read,
-    settingsCache = require('../settings/cache'),
-    updateThemeList,
     loadAllThemes,
-    loadOneTheme,
+    loadOneTheme;
-    initThemes;
-
-updateThemeList = function updateThemeList(themes) {
-    debug('loading themes', Object.keys(themes));
-    themeList.init(themes);
-};
 
 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
 };

core/server/themes/read.js

@@ -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;

core/test/unit/middleware/theme-handler_spec.js

@@ -1,14 +1,13 @@
-var sinon        = require('sinon'),
+var sinon = require('sinon'),
-    should       = require('should'),
+    should = require('should'),
-    express      = require('express'),
+    express = require('express'),
-    fs           = require('fs'),
+    fs = require('fs'),
-    hbs          = require('express-hbs'),
+    hbs = require('express-hbs'),
-    themeList   = require('../../../server/themes').list,
+    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();
-            });
-        });
     });
 });

core/test/unit/themes_spec.js

@@ -1,11 +1,12 @@
-var should          = require('should'),
+var should = require('should'),
-    sinon           = require('sinon'),
+    sinon = require('sinon'),
-    _               = require('lodash'),
+    _ = require('lodash'),
-    fs              = require('fs'),
+    fs = require('fs'),
-    tmp             = require('tmp'),
+    tmp = require('tmp'),
-    join            = require('path').join,
+    join = require('path').join,
-    themeList      = require('../../server/themes').list,
+    config = require('../../server/config'),
-    readThemes      = require('../../server/themes/read'),
+    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 () {
+    describe('Loader', function () {
-        sandbox.restore();
+        var themePath;
-    });
 
-    describe('Read All', function () {
+        beforeEach(function () {
-        it('should read directory and include only folders', function (done) {
+            themePath = tmp.dirSync({unsafeCleanup: true});
-            var themePath = tmp.dirSync({unsafeCleanup: true});
+            sandbox.stub(config, 'getContentPath').withArgs('themes').returns(themePath.name);
-
-            // 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);
         });
 
-        it('should read directory and read package.json if present', function (done) {
+        afterEach(function () {
-            var themePath = tmp.dirSync({unsafeCleanup: true});
+            themePath.removeCallback();
+            sandbox.restore();
+        });
 
-            // create trash
+        describe('Load All', function () {
-            fs.writeFileSync(join(themePath.name, 'README.md'));
+            it('should load directory and include only folders', function (done) {
-            fs.writeFileSync(join(themePath.name, 'Thumbs.db'));
+                // create trash
+                fs.writeFileSync(join(themePath.name, 'casper.zip'));
+                fs.writeFileSync(join(themePath.name, '.DS_Store'));
 
-            // create actual theme
+                // create actual theme
-            fs.mkdirSync(join(themePath.name, 'casper'));
+                fs.mkdirSync(join(themePath.name, 'casper'));
-            fs.mkdirSync(join(themePath.name, 'not-casper'));
+                fs.mkdirSync(join(themePath.name, 'casper', 'partials'));
-            fs.writeFileSync(
+                fs.writeFileSync(join(themePath.name, 'casper', 'index.hbs'));
-                join(themePath.name, 'casper', 'package.json'),
+                fs.writeFileSync(join(themePath.name, 'casper', 'partials', 'navigation.hbs'));
-                JSON.stringify({name: 'casper', version: '0.1.2'})
-            );
 
-            readThemes.all(themePath.name)
+                themes.loadAll()
-                .then(function (themeList) {
+                    .then(function (result) {
-                    themeList.should.eql({
+                        var themeResult = themeList.getAll();
-                        casper: {
+
+                        // 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',
+                        done();
-                            path: join(themePath.name, 'not-casper'),
+                    })
-                            'package.json': null
+                    .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);
         });
     });