Use "mounting" concept for active theme (#8193)

no issue

🔥 Remove DIRTY HACK for API
- this is no longer needed, because themes get mounted in every case

 Switch to concept of 'mounted' theme
- check if active theme is mounted
- if not, mount it
- mounting is a function OF the active theme

🎨 Move theme middleware to theme module

🎨 Update theme middleware function names
- update the function names and comments to be more representative of their current functions
- this was pretty old and out of date!

🚨 Fixup tests for middleware
- ensure the objects match what we expect
- based partially on theme docs

Update TODO
This commit is contained in:
Hannah Wolfe 2017-03-21 09:03:09 +00:00 committed by Katharina Irrgang
parent 47e00900cc
commit 495eee7747
8 changed files with 430 additions and 295 deletions

View File

@ -76,9 +76,6 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) {
if (isActiveThemeUpdate(method, endpoint, result)) {
// Special case for if we're overwriting an active theme
// @TODO: remove this crazy DIRTY HORRIBLE HACK
req.app.set('activeTheme', null);
config.set('assetHash', null);
return INVALIDATE_ALL;
} else if (['POST', 'PUT', 'DELETE'].indexOf(method) > -1) {
if (endpoint === 'schedules' && subdir === 'posts') {

View File

@ -20,9 +20,11 @@ var debug = require('debug')('ghost:blog'),
prettyURLs = require('../middleware/pretty-urls'),
serveSharedFile = require('../middleware/serve-shared-file'),
staticTheme = require('../middleware/static-theme'),
themeHandler = require('../middleware/theme-handler'),
customRedirects = require('../middleware/custom-redirects'),
serveFavicon = require('../middleware/serve-favicon');
serveFavicon = require('../middleware/serve-favicon'),
// middleware for themes
themeMiddleware = require('../themes').middleware;
module.exports = function setupBlogApp() {
debug('Blog setup start');
@ -55,8 +57,7 @@ module.exports = function setupBlogApp() {
// 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);
blogApp.use(themeMiddleware);
debug('Themes done');
// Theme static assets/files

View File

@ -1,112 +0,0 @@
var _ = require('lodash'),
hbs = require('express-hbs'),
config = require('../config'),
utils = require('../utils'),
errors = require('../errors'),
i18n = require('../i18n'),
settingsCache = require('../settings/cache'),
themeUtils = require('../themes'),
themeHandler;
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 blogData = {
title: settingsCache.get('title'),
description: settingsCache.get('description'),
facebook: settingsCache.get('facebook'),
twitter: settingsCache.get('twitter'),
timezone: settingsCache.get('activeTimezone'),
navigation: settingsCache.get('navigation'),
icon: settingsCache.get('icon'),
cover: settingsCache.get('cover'),
logo: settingsCache.get('logo'),
amp: settingsCache.get('amp')
},
labsData = _.cloneDeep(settingsCache.get('labs')),
themeData = {};
if (themeUtils.getActive()) {
themeData.posts_per_page = themeUtils.getActive().config('posts_per_page');
}
// Request-specific information
// These things are super dependent on the request, so they need to be in middleware
blogData.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: blogData,
labs: labsData,
config: themeData
}
});
next();
},
// ### Activate Theme
// Helper for updateActiveTheme
activateTheme: function activateTheme(blogApp) {
var hbsOptions = {
partialsDir: [config.get('paths').helperTemplates],
onCompile: function onCompile(exhbs, source) {
return exhbs.handlebars.compile(source, {preventIndent: true});
}
};
if (themeUtils.getActive().hasPartials()) {
hbsOptions.partialsDir.push(themeUtils.getActive().partialsPath);
}
// reset the asset hash
config.set('assetHash', null);
// clear the view cache
blogApp.cache = {};
// Set the views and engine
blogApp.set('views', themeUtils.getActive().path);
blogApp.engine('hbs', hbs.express3(hbsOptions));
// Set active theme variable on the express server
// Note: this is effectively the "mounted" theme, which has been loaded into the express app
blogApp.set('activeTheme', themeUtils.getActive().name);
},
// ### updateActiveTheme
// Updates the blogApp's activeTheme variable and subsequently
// activates that theme's views with the hbs templating engine if it
// is not yet activated.
updateActiveTheme: function updateActiveTheme(req, res, next) {
var blogApp = req.app,
// We use the settingsCache here, because the setting will be set, even if the theme itself is
// not usable because it is invalid or missing.
activeThemeName = settingsCache.get('activeTheme'),
mountedThemeName = blogApp.get('activeTheme');
// This means that the theme hasn't been loaded yet i.e. there is no active theme
if (!themeUtils.getActive()) {
// 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);
}
next();
}
};
module.exports = themeHandler;

View File

@ -19,6 +19,9 @@
var _ = require('lodash'),
join = require('path').join,
defaultConfig = require('./defaults.json'),
config = require('../config'),
// @TODO: remove this require
hbs = require('express-hbs'),
// Current instance of ActiveTheme
currentActiveTheme;
@ -44,6 +47,7 @@ class ActiveTheme {
// Assign some data, mark it all as pseudo-private
this._name = loadedTheme.name;
this._path = loadedTheme.path;
this._mounted = false;
// @TODO: get gscan to return validated, useful package.json fields for us!
this._packageInfo = loadedTheme['package.json'];
@ -73,6 +77,10 @@ class ActiveTheme {
return join(this.path, 'partials');
}
get mounted() {
return this._mounted;
}
hasPartials() {
return this._partials.length > 0;
}
@ -84,6 +92,30 @@ class ActiveTheme {
config(key) {
return this._config[key];
}
mount(blogApp) {
let hbsOptions = {
partialsDir: [config.get('paths').helperTemplates],
onCompile: function onCompile(exhbs, source) {
return exhbs.handlebars.compile(source, {preventIndent: true});
}
};
if (this.hasPartials()) {
hbsOptions.partialsDir.push(this.partialsPath);
}
// reset the asset hash
// @TODO: set this on the theme instead of globally, or use proper file-based hash
config.set('assetHash', null);
// clear the view cache
blogApp.cache = {};
// Set the views and engine
blogApp.set('views', this.path);
blogApp.engine('hbs', hbs.express3(hbsOptions));
this._mounted = true;
}
}
module.exports = {

View File

@ -63,5 +63,6 @@ module.exports = {
// Use the two theme objects to set the current active theme
active.set(loadedTheme, checkedTheme);
}
},
middleware: require('./middleware')
};

View File

@ -0,0 +1,81 @@
var _ = require('lodash'),
hbs = require('express-hbs'),
utils = require('../utils'),
errors = require('../errors'),
i18n = require('../i18n'),
settingsCache = require('../settings/cache'),
activeTheme = require('./active'),
themeMiddleware = {};
// ### Ensure Active Theme
// Ensure there's a properly set & mounted active theme before attempting to serve a blog request
// If there is no active theme, throw an error
// Else, ensure the active theme is mounted
themeMiddleware.ensureActiveTheme = function ensureActiveTheme(req, res, next) {
// This means that the theme hasn't been loaded yet i.e. there is no active theme
if (!activeTheme.get()) {
// 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({
// We use the settingsCache here, because the setting will be set,
// even if the theme itself is not usable because it is invalid or missing.
message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: settingsCache.get('activeTheme')})
}));
}
// If the active theme has not yet been mounted, mount it into express
if (!activeTheme.get().mounted) {
activeTheme.get().mount(req.app);
}
next();
};
// ### Update Template Data
// Updates handlebars with the contextual data for the current request
themeMiddleware.updateTemplateData = function updateTemplateData(req, res, next) {
// Static information, same for every request unless the settings change
// @TODO: bind this once and then update based on events?
var blogData = {
title: settingsCache.get('title'),
description: settingsCache.get('description'),
facebook: settingsCache.get('facebook'),
twitter: settingsCache.get('twitter'),
timezone: settingsCache.get('activeTimezone'),
navigation: settingsCache.get('navigation'),
permalinks: settingsCache.get('permalinks'),
icon: settingsCache.get('icon'),
cover: settingsCache.get('cover'),
logo: settingsCache.get('logo'),
amp: settingsCache.get('amp')
},
labsData = _.cloneDeep(settingsCache.get('labs')),
themeData = {};
if (activeTheme.get()) {
themeData.posts_per_page = activeTheme.get().config('posts_per_page');
}
// Request-specific information
// These things are super dependent on the request, so they need to be in middleware
blogData.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: blogData,
labs: labsData,
config: themeData
}
});
next();
};
module.exports = [
themeMiddleware.ensureActiveTheme,
themeMiddleware.updateTemplateData
];

View File

@ -1,174 +0,0 @@
var should = require('should'),
sinon = require('sinon'),
express = require('express'),
hbs = require('express-hbs'),
configUtils = require('../../utils/configUtils'),
themeUtils = require('../../../server/themes'),
themeList = themeUtils.list,
themeHandler = require('../../../server/middleware/theme-handler'),
settingsCache = require('../../../server/settings/cache'),
sandbox = sinon.sandbox.create();
describe('Theme Handler', function () {
var req, res, blogApp, getActiveThemeStub;
beforeEach(function () {
req = sinon.spy();
res = sinon.spy();
blogApp = express();
req.app = blogApp;
getActiveThemeStub = sandbox.stub(themeUtils, 'getActive').returns({
config: sandbox.stub()
});
});
afterEach(function () {
sandbox.restore();
themeList.init();
});
describe('activateTheme', function () {
var hbsStub;
beforeEach(function () {
hbsStub = sandbox.spy(hbs, 'express3');
});
it('should activate new theme with partials', function () {
getActiveThemeStub.returns({
name: 'casper',
path: 'my/fake/path',
partialsPath: 'my/fake/path/partials',
hasPartials: function () {return true;}
});
themeHandler.activateTheme(blogApp);
// hasPartials, partialsPath, path & name
getActiveThemeStub.callCount.should.be.eql(4);
hbsStub.calledOnce.should.be.true();
hbsStub.firstCall.args[0].should.be.an.Object().and.have.property('partialsDir');
hbsStub.firstCall.args[0].partialsDir.should.be.an.Array().with.lengthOf(2);
hbsStub.firstCall.args[0].partialsDir[1].should.eql('my/fake/path/partials');
// Check the asset hash gets reset
should(configUtils.config.get('assetHash')).eql(null);
blogApp.get('activeTheme').should.equal('casper');
blogApp.get('views').should.eql('my/fake/path');
});
it('should activate new theme without partials', function () {
getActiveThemeStub.returns({
name: 'casper',
path: 'my/fake/path',
hasPartials: function () {return false;}
});
themeHandler.activateTheme(blogApp);
// hasPartials, path & name
getActiveThemeStub.callCount.should.eql(3);
hbsStub.calledOnce.should.be.true();
hbsStub.firstCall.args[0].should.be.an.Object().and.have.property('partialsDir');
hbsStub.firstCall.args[0].partialsDir.should.have.lengthOf(1);
// Check the asset hash gets reset
should(configUtils.config.get('assetHash')).eql(null);
blogApp.get('activeTheme').should.equal('casper');
blogApp.get('views').should.eql('my/fake/path');
});
});
// NOTE: These tests are totally dependent on the previous tests
// @TODO: properly fix these tests once theme refactor is finished
describe('configHbsForContext', function () {
var updateOptionsSpy;
beforeEach(function () {
updateOptionsSpy = sandbox.spy(hbs, 'updateTemplateOptions');
});
it('handles non secure context', function (done) {
res.locals = {};
themeHandler.configHbsForContext(req, res, function next() {
updateOptionsSpy.calledOnce.should.be.true();
should.not.exist(res.locals.secure);
done();
});
});
it('handles secure context', function (done) {
req.secure = true;
res.locals = {};
themeHandler.configHbsForContext(req, res, function next() {
updateOptionsSpy.calledOnce.should.be.true();
should.exist(res.locals.secure);
res.locals.secure.should.be.true();
done();
});
});
});
describe('updateActiveTheme', function () {
var activateThemeStub,
settingsCacheStub;
beforeEach(function () {
activateThemeStub = sandbox.stub(themeHandler, 'activateTheme');
settingsCacheStub = sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('casper');
});
it('updates the active theme if changed', function (done) {
blogApp.set('activeTheme', 'not-casper');
themeHandler.updateActiveTheme(req, res, function next(err) {
// Did not throw an error
should.not.exist(err);
settingsCacheStub.calledWith('activeTheme').should.be.true();
getActiveThemeStub.called.should.be.true();
activateThemeStub.called.should.be.true();
activateThemeStub.calledWith(blogApp).should.be.true();
done();
});
});
it('does not update the active theme if not changed', function (done) {
blogApp.set('activeTheme', 'casper');
themeHandler.updateActiveTheme(req, res, function next(err) {
// Did not throw an error
should.not.exist(err);
settingsCacheStub.calledWith('activeTheme').should.be.true();
getActiveThemeStub.called.should.be.true();
activateThemeStub.called.should.be.false();
done();
});
});
it('throws error if theme is missing', function (done) {
getActiveThemeStub.returns(undefined);
themeHandler.updateActiveTheme(req, res, function next(err) {
// Did throw an error
should.exist(err);
err.message.should.eql('The currently active theme "casper" is missing.');
settingsCacheStub.calledWith('activeTheme').should.be.true();
getActiveThemeStub.called.should.be.true();
activateThemeStub.called.should.be.false();
done();
});
});
});
});

View File

@ -4,13 +4,23 @@ var should = require('should'), // jshint ignore:line
fs = require('fs'),
tmp = require('tmp'),
join = require('path').join,
hbs = require('express-hbs'),
config = require('../../server/config'),
themes = require('../../server/themes'),
// is only exposed via themes.getActive()
activeTheme = require('../../server/themes/active'),
settingsCache = require('../../server/settings/cache'),
themeList = themes.list,
middleware = themes.middleware,
sandbox = sinon.sandbox.create();
describe('Themes', function () {
afterEach(function () {
sandbox.restore();
});
describe('Loader', function () {
var themePath;
@ -21,7 +31,6 @@ describe('Themes', function () {
afterEach(function () {
themePath.removeCallback();
sandbox.restore();
});
describe('Load All', function () {
@ -209,4 +218,304 @@ describe('Themes', function () {
Object.keys(result).should.have.length(0);
});
});
describe('Middleware', function () {
var req, res, blogApp, getActiveThemeStub, settingsCacheStub;
beforeEach(function () {
req = sandbox.spy();
res = sandbox.spy();
blogApp = {test: 'obj'};
req.app = blogApp;
res.locals = {};
getActiveThemeStub = sandbox.stub(activeTheme, 'get');
settingsCacheStub = sandbox.stub(settingsCache, 'get');
});
describe('ensureActiveTheme', function () {
var ensureActiveTheme = middleware[0],
mountThemeSpy;
beforeEach(function () {
mountThemeSpy = sandbox.spy();
settingsCacheStub.withArgs('activeTheme').returns('casper');
});
it('mounts active theme if not yet mounted', function (done) {
getActiveThemeStub.returns({
mounted: false,
mount: mountThemeSpy
});
ensureActiveTheme(req, res, function next(err) {
// Did not throw an error
should.not.exist(err);
settingsCacheStub.called.should.be.false();
getActiveThemeStub.called.should.be.true();
mountThemeSpy.called.should.be.true();
mountThemeSpy.calledWith(blogApp).should.be.true();
done();
});
});
it('does not mounts the active theme if it is already mounted', function (done) {
getActiveThemeStub.returns({
mounted: true,
mount: mountThemeSpy
});
ensureActiveTheme(req, res, function next(err) {
// Did not throw an error
should.not.exist(err);
settingsCacheStub.called.should.be.false();
getActiveThemeStub.called.should.be.true();
mountThemeSpy.called.should.be.false();
done();
});
});
it('throws error if theme is missing', function (done) {
getActiveThemeStub.returns(undefined);
ensureActiveTheme(req, res, function next(err) {
// Did throw an error
should.exist(err);
err.message.should.eql('The currently active theme "casper" is missing.');
settingsCacheStub.calledWith('activeTheme').should.be.true();
getActiveThemeStub.called.should.be.true();
mountThemeSpy.called.should.be.false();
done();
});
});
});
describe('updateTemplateData', function () {
var updateTemplateData = middleware[1],
themeDataExpectedProps = ['posts_per_page'],
blogDataExpectedProps = [
'url', 'title', 'description', 'logo', 'cover', 'icon', 'twitter', 'facebook', 'navigation',
'permalinks', 'timezone', 'amp'
],
updateOptionsStub;
beforeEach(function () {
updateOptionsStub = sandbox.stub(hbs, 'updateTemplateOptions');
settingsCacheStub.withArgs('title').returns('Bloggy McBlogface');
settingsCacheStub.withArgs('labs').returns({});
getActiveThemeStub.returns({
config: sandbox.stub().returns(2)
});
});
it('calls updateTemplateOptions with correct data', function (done) {
updateTemplateData(req, res, function next(err) {
var templateOptions;
should.not.exist(err);
updateOptionsStub.calledOnce.should.be.true();
templateOptions = updateOptionsStub.firstCall.args[0];
templateOptions.should.be.an.Object().with.property('data');
templateOptions.data.should.be.an.Object().with.properties('blog', 'labs', 'config');
// Check Theme Config
templateOptions.data.config.should.be.an.Object()
.with.properties(themeDataExpectedProps)
.and.size(themeDataExpectedProps.length);
// posts per page should be set according to the stub
templateOptions.data.config.posts_per_page.should.eql(2);
// Check blog config
// blog should have all the right properties
templateOptions.data.blog.should.be.an.Object()
.with.properties(blogDataExpectedProps)
.and.size(blogDataExpectedProps.length);
// url should be correct
templateOptions.data.blog.url.should.eql('http://127.0.0.1:2369/');
// should get the title
templateOptions.data.blog.title.should.eql('Bloggy McBlogface');
// Check labs config
templateOptions.data.labs.should.be.an.Object();
// Check res.locals
should.not.exist(res.locals.secure);
done();
});
});
it('does not error if there is no active theme', function (done) {
getActiveThemeStub.returns(undefined);
updateTemplateData(req, res, function next(err) {
var templateOptions;
should.not.exist(err);
updateOptionsStub.calledOnce.should.be.true();
templateOptions = updateOptionsStub.firstCall.args[0];
templateOptions.should.be.an.Object().with.property('data');
templateOptions.data.should.be.an.Object().with.properties('blog', 'labs', 'config');
// Check Theme Config
templateOptions.data.config.should.be.an.Object();
// posts per page should NOT be set as there's no active theme
should.not.exist(templateOptions.data.config.posts_per_page);
// Check blog config
// blog should have all the right properties
templateOptions.data.blog.should.be.an.Object()
.with.properties(blogDataExpectedProps)
.and.size(blogDataExpectedProps.length);
// url should be correct
templateOptions.data.blog.url.should.eql('http://127.0.0.1:2369/');
// should get the title
templateOptions.data.blog.title.should.eql('Bloggy McBlogface');
// Check labs config
templateOptions.data.labs.should.be.an.Object();
done();
});
});
it('calls updateTempalateOptions with correct info for secure context', function (done) {
req.secure = true;
updateTemplateData(req, res, function next(err) {
var templateOptions;
should.not.exist(err);
updateOptionsStub.calledOnce.should.be.true();
templateOptions = updateOptionsStub.firstCall.args[0];
templateOptions.should.be.an.Object().with.property('data');
templateOptions.data.should.be.an.Object().with.properties('blog', 'labs', 'config');
// Check Theme Config
templateOptions.data.config.should.be.an.Object()
.with.properties(themeDataExpectedProps)
.and.size(themeDataExpectedProps.length);
// posts per page should be set according to the stub
templateOptions.data.config.posts_per_page.should.eql(2);
// Check blog config
// blog should have all the right properties
templateOptions.data.blog.should.be.an.Object()
.with.properties(blogDataExpectedProps)
.and.size(blogDataExpectedProps.length);
// url should be correct HTTPS!
templateOptions.data.blog.url.should.eql('https://127.0.0.1:2369/');
// should get the title
templateOptions.data.blog.title.should.eql('Bloggy McBlogface');
// Check labs config
templateOptions.data.labs.should.be.an.Object();
// Check res.locals
should.exist(res.locals.secure);
res.locals.secure.should.be.true();
done();
});
});
});
});
describe('Active', function () {
describe('Mount', function () {
var hbsStub, configStub,
fakeBlogApp, fakeLoadedTheme, fakeCheckedTheme;
beforeEach(function () {
hbsStub = sandbox.stub(hbs, 'express3');
configStub = sandbox.stub(config, 'set');
fakeBlogApp = {
cache: ['stuff'],
set: sandbox.stub(),
engine: sandbox.stub()
};
fakeLoadedTheme = {
name: 'casper',
path: 'my/fake/theme/path'
};
fakeCheckedTheme = {};
});
it('should mount active theme with partials', function () {
// setup partials
fakeCheckedTheme.partials = ['loop', 'navigation'];
var theme = activeTheme.set(fakeLoadedTheme, fakeCheckedTheme);
// Check the theme is not yet mounted
activeTheme.get().mounted.should.be.false();
// Call mount!
theme.mount(fakeBlogApp);
// Check the asset hash gets reset
configStub.calledOnce.should.be.true();
configStub.calledWith('assetHash', null).should.be.true();
// Check te view cache was cleared
fakeBlogApp.cache.should.eql({});
// Check the views were set correctly
fakeBlogApp.set.calledOnce.should.be.true();
fakeBlogApp.set.calledWith('views', 'my/fake/theme/path').should.be.true();
// Check handlebars was initialised correctly
hbsStub.calledOnce.should.be.true();
hbsStub.firstCall.args[0].should.be.an.Object().and.have.property('partialsDir');
hbsStub.firstCall.args[0].partialsDir.should.be.an.Array().with.lengthOf(2);
hbsStub.firstCall.args[0].partialsDir[1].should.eql('my/fake/theme/path/partials');
// Check the theme is now mounted
activeTheme.get().mounted.should.be.true();
});
it('should mount active theme without partials', function () {
// setup partials
fakeCheckedTheme.partials = [];
var theme = activeTheme.set(fakeLoadedTheme, fakeCheckedTheme);
// Check the theme is not yet mounted
activeTheme.get().mounted.should.be.false();
// Call mount!
theme.mount(fakeBlogApp);
// Check the asset hash gets reset
configStub.calledOnce.should.be.true();
configStub.calledWith('assetHash', null).should.be.true();
// Check te view cache was cleared
fakeBlogApp.cache.should.eql({});
// Check the views were set correctly
fakeBlogApp.set.calledOnce.should.be.true();
fakeBlogApp.set.calledWith('views', 'my/fake/theme/path').should.be.true();
// Check handlebars was initialised correctly
hbsStub.calledOnce.should.be.true();
hbsStub.firstCall.args[0].should.be.an.Object().and.have.property('partialsDir');
hbsStub.firstCall.args[0].partialsDir.should.have.lengthOf(1);
// Check the theme is now mounted
activeTheme.get().mounted.should.be.true();
});
});
});
});