From fef9b4be25e441d7094ae127136d8ceab3a73c4d Mon Sep 17 00:00:00 2001 From: Seb Gotvitch Date: Sun, 1 Dec 2013 18:31:55 -0500 Subject: [PATCH] Bug fixes for partial views closes #1203 - Update express-hbs module to the new version (0.5.2) - Use two instance of hbs one for the theme and an other for the admin - Template helpers are register as partial view - Partial views of the theme are reload when the theme changed Remove clear partial cache in handlebars This code will be move in `express-hbs`. This doesn't cause a problem to remove this line but it is not clean. Remove unused hbs instance Resolve conflict --- core/server/helpers/index.js | 62 +++++--- core/server/helpers/template.js | 45 ++---- core/server/index.js | 27 ++-- core/server/middleware/index.js | 34 ++--- core/test/unit/helpers_template_spec.js | 95 +----------- core/test/unit/server_helpers_index_spec.js | 154 +++++++++----------- package.json | 2 +- 7 files changed, 166 insertions(+), 253 deletions(-) diff --git a/core/server/helpers/index.js b/core/server/helpers/index.js index a34e34c60c7..814c87bbd0a 100644 --- a/core/server/helpers/index.js +++ b/core/server/helpers/index.js @@ -17,6 +17,8 @@ var _ = require('underscore'), coreHelpers = {}, registerHelpers; + + /** * [ description] * @todo ghost core helpers + a way for themes to register them @@ -536,10 +538,6 @@ coreHelpers.has_tag = function (name, options) { return options.inverse(this); }; -// ## Template driven helpers -// Template driven helpers require that their template is loaded before they can be registered. -coreHelpers.paginationTemplate = null; - // ### Pagination Helper // `{{pagination}}` // Outputs previous and next buttons, along with info about the current page @@ -564,7 +562,7 @@ coreHelpers.pagination = function (options) { errors.logAndThrowError('Invalid value, check page, pages, limit and total are numbers'); return; } - return new hbs.handlebars.SafeString(coreHelpers.paginationTemplate(this.pagination)); + return template.execute('pagination', this.pagination); }; coreHelpers.helperMissing = function (arg) { @@ -574,13 +572,8 @@ coreHelpers.helperMissing = function (arg) { errors.logError('Missing helper: "' + arg + '"'); }; -// Register a handlebars helper for themes -function registerThemeHelper(name, fn) { - hbs.registerHelper(name, fn); -} - -// Register an async handlebars helper for themes -function registerAsyncThemeHelper(name, fn) { +// Register an async handlebars helper for a given handlebars instance +function registerAsyncHelper(hbs, name, fn) { hbs.registerAsyncHelper(name, function (options, cb) { // Wrap the function passed in with a when.resolve so it can // return either a promise or a value @@ -592,12 +585,39 @@ function registerAsyncThemeHelper(name, fn) { }); } -registerHelpers = function (config) { - var paginationHelper; + +// Register a handlebars helper for themes +function registerThemeHelper(name, fn) { + hbs.registerHelper(name, fn); +} + +// Register an async handlebars helper for themes +function registerAsyncThemeHelper(name, fn) { + registerAsyncHelper(hbs, name, fn); +} + +// Register a handlebars helper for admin +function registerAdminHelper(name, fn) { + coreHelpers.adminHbs.registerHelper(name, fn); +} + +// Register an async handlebars helper for admin +function registerAsyncAdminHelper(name, fn) { + registerAsyncHelper(coreHelpers.adminHbs, name, fn); +} + + + +registerHelpers = function (config, adminHbs) { // And expose config coreHelpers.config = config; + // Expose hbs instance for admin + coreHelpers.adminHbs = adminHbs; + + + // Register theme helpers registerThemeHelper('asset', coreHelpers.asset); registerThemeHelper('author', coreHelpers.author); @@ -622,6 +642,8 @@ registerHelpers = function (config) { registerThemeHelper('pageUrl', coreHelpers.pageUrl); + registerThemeHelper('pagination', coreHelpers.pagination); + registerThemeHelper('tags', coreHelpers.tags); registerAsyncThemeHelper('body_class', coreHelpers.body_class); @@ -640,18 +662,14 @@ registerHelpers = function (config) { registerAsyncThemeHelper('url', coreHelpers.url); - paginationHelper = template.loadTemplate('pagination').then(function (templateFn) { - coreHelpers.paginationTemplate = templateFn; - registerThemeHelper('pagination', coreHelpers.pagination); - }); + // Register admin helpers + registerAdminHelper('asset', coreHelpers.asset); - // Return once the template-driven helpers have loaded - return when.join( - paginationHelper - ); + registerAdminHelper('ghostScriptTags', coreHelpers.ghostScriptTags); }; + module.exports = coreHelpers; module.exports.loadCoreHelpers = registerHelpers; module.exports.registerThemeHelper = registerThemeHelper; diff --git a/core/server/helpers/template.js b/core/server/helpers/template.js index deeb7278bf3..3dcdd9a9d8f 100644 --- a/core/server/helpers/template.js +++ b/core/server/helpers/template.js @@ -1,42 +1,27 @@ var templates = {}, - nodefn = require('when/node/function'), - fs = require('fs'), hbs = require('express-hbs'), - errors = require('../errorHandling'), - path = require('path'), - when = require('when'), - config = require('../config'), - api = require('../api'); + errors = require('../errorHandling'); // ## Template utils -// Compile a template for a handlebars helper -templates.compileTemplate = function (templatePath) { - return nodefn.call(fs.readFile, templatePath).then(function (templateContents) { - return hbs.handlebars.compile(templateContents.toString()); - }, errors.logAndThrowError); -}; +// Execute a template helper +// All template helpers are register as partial view. +templates.execute = function (name, context) { -// Load a template for a handlebars helper -templates.loadTemplate = function (name) { - var templateFileName = name + '.hbs', - deferred = when.defer(); - // Check for theme specific version first - return api.settings.read('activeTheme').then(function (activeTheme) { - var templatePath = path.join(config.paths().themePath, activeTheme.value, 'partials', templateFileName); + var partial = hbs.handlebars.partials[name]; - // Can't use nodefn here because exists just returns one parameter, true or false - fs.exists(templatePath, function (exists) { - if (!exists) { - // Fall back to helpers templates location - templatePath = path.join(config.paths().helperTemplates, templateFileName); - } + if (partial === undefined) { + errors.logAndThrowError('Template ' + name + ' not found.'); + return; + } - templates.compileTemplate(templatePath).then(deferred.resolve, deferred.reject); - }); + // If the partial view is not compiled, it compiles and saves in handlebars + if (typeof partial === 'string') { + partial = hbs.handlebars.compile(partial); + hbs.handlebars.partials[name] = partial; + } - return deferred.promise; - }); + return new hbs.handlebars.SafeString(partial(context)); }; module.exports = templates; \ No newline at end of file diff --git a/core/server/index.js b/core/server/index.js index 71f1c04b040..6f35b8afa31 100644 --- a/core/server/index.js +++ b/core/server/index.js @@ -22,6 +22,7 @@ var config = require('./config'), permissions = require('./permissions'), uuid = require('node-uuid'), api = require('./api'), + hbs = require('express-hbs'), // Variables setup, @@ -105,18 +106,14 @@ function setup(server) { permissions.init() ); }).then(function () { - return when.join( - // Initialise mail after first run, - // passing in config module to prevent - // circular dependencies. - mailer.init(), - helpers.loadCoreHelpers(config) - ); + // Initialise mail after first run, + // passing in config module to prevent + // circular dependencies. + return mailer.init(); }).then(function () { + var adminHbs; // ##Configuration - // set the view engine - server.set('view engine', 'hbs'); // set the configured URL server.set('ghost root', config.theme().path); @@ -124,6 +121,18 @@ function setup(server) { // return the correct mime type for woff filess express['static'].mime.define({'application/font-woff': ['woff']}); + // ## View engine + // set the view engine + server.set('view engine', 'hbs'); + + // Create a hbs instance for admin and init view engine + adminHbs = hbs.create(); + server.set('admin view engine', adminHbs.express3({partialsDir: config.paths().adminViews + 'partials'})); + + // Load helpers + helpers.loadCoreHelpers(config, adminHbs); + + // ## Middleware middleware(server, dbHash); diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 2a780f47df3..2b1e3843a23 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -70,22 +70,13 @@ function ghostLocals(req, res, next) { // Initialise Theme or Admin Views function initViews(req, res, next) { /*jslint unparam:true*/ - var hbsOptions; if (!res.isAdmin) { - // self.globals is a hack til we have a better way of getting combined settings & config - hbsOptions = {templateOptions: {data: {blog: config.theme()}}}; - api.settings.read('activeTheme').then(function (activeTheme) { - if (config.paths().availableThemes[activeTheme.value].hasOwnProperty('partials')) { - // Check that the theme has a partials directory before trying to use it - hbsOptions.partialsDir = path.join(config.paths().themePath, activeTheme.value, 'partials'); - } - - expressServer.engine('hbs', hbs.express3(hbsOptions)); - expressServer.set('views', path.join(config.paths().themePath, activeTheme.value)); - }); + hbs.updateTemplateOptions({ data: {blog: config.theme()} }); + expressServer.engine('hbs', expressServer.get('theme view engine')); + expressServer.set('views', path.join(config.paths().themePath, expressServer.get('activeTheme'))); } else { - expressServer.engine('hbs', hbs.express3({partialsDir: config.paths().adminViews + 'partials'})); + expressServer.engine('hbs', expressServer.get('admin view engine')); expressServer.set('views', config.paths().adminViews); } @@ -95,9 +86,10 @@ function initViews(req, res, next) { // ### Activate Theme // Helper for manageAdminAndTheme function activateTheme(activeTheme) { - var stackLocation = _.indexOf(expressServer.stack, _.find(expressServer.stack, function (stackItem) { - return stackItem.route === '' && stackItem.handle.name === 'settingEnabled'; - })); + var hbsOptions, + stackLocation = _.indexOf(expressServer.stack, _.find(expressServer.stack, function (stackItem) { + return stackItem.route === '' && stackItem.handle.name === 'settingEnabled'; + })); // clear the view cache expressServer.cache = {}; @@ -108,6 +100,14 @@ function activateTheme(activeTheme) { expressServer.stack[stackLocation].handle = middleware.whenEnabled(expressServer.get('activeTheme'), middleware.staticTheme()); } + // set view engine + hbsOptions = { partialsDir: [ config.paths().helperTemplates ] }; + if (config.paths().availableThemes[activeTheme].hasOwnProperty('partials')) { + // Check that the theme has a partials directory before trying to use it + hbsOptions.partialsDir.push(path.join(config.paths().themePath, activeTheme, 'partials')); + } + expressServer.set('theme view engine', hbs.express3(hbsOptions)); + // Update user error template errors.updateActiveTheme(activeTheme); } @@ -242,4 +242,4 @@ module.exports = function (server, dbHash) { // Export middleware functions directly module.exports.middleware = middleware; // Expose middleware functions in this file as well -module.exports.middleware.redirectToSignup = redirectToSignup; \ No newline at end of file +module.exports.middleware.redirectToSignup = redirectToSignup; diff --git a/core/test/unit/helpers_template_spec.js b/core/test/unit/helpers_template_spec.js index dd82697ce59..21c4f09479c 100644 --- a/core/test/unit/helpers_template_spec.js +++ b/core/test/unit/helpers_template_spec.js @@ -5,6 +5,7 @@ var testUtils = require('../utils'), when = require('when'), _ = require('underscore'), path = require('path'), + hbs = require('express-hbs'), // Stuff we are testing config = require('../../server/config'), @@ -13,96 +14,12 @@ var testUtils = require('../utils'), describe('Helpers Template', function () { - var testTemplatePath = 'core/test/utils/fixtures/', - sandbox; + it("can execute a template", function () { + hbs.registerPartial('test', '

Hello {{name}}

'); - beforeEach(function () { - sandbox = sinon.sandbox.create(); - }); - - afterEach(function () { - sandbox.restore(); - }); - - it("can compile a template", function (done) { - var testTemplate = path.join(process.cwd(), testTemplatePath, 'test.hbs'); - - should.exist(template.compileTemplate, 'Template Compiler exists'); - - template.compileTemplate(testTemplate).then(function (templateFn) { - should.exist(templateFn); - _.isFunction(templateFn).should.equal(true); - - templateFn().should.equal('

HelloWorld

'); - done(); - }).then(null, done); - }); - - it("loads templates for helpers", function (done) { - var compileSpy = sandbox.spy(template, 'compileTemplate'), - pathsStub; - - should.exist(template.loadTemplate, 'load template function exists'); - - // In order for the test to work, need to replace the path to the template - pathsStub = sandbox.stub(config, "paths", function () { - return { - // Forcing the theme path to be the same - themePath: path.join(process.cwd(), testTemplatePath), - helperTemplates: path.join(process.cwd(), testTemplatePath) - }; - }); - apiStub = sandbox.stub(api.settings , 'read', function () { - return when({value: 'casper'}); - }); - - template.loadTemplate('test').then(function (templateFn) { - compileSpy.restore(); - pathsStub.restore(); - // test that compileTemplate was called with the expected path - compileSpy.calledOnce.should.equal(true); - compileSpy.calledWith(path.join(process.cwd(), testTemplatePath, 'test.hbs')).should.equal(true); - should.exist(templateFn); - _.isFunction(templateFn).should.equal(true); - - templateFn().should.equal('

HelloWorld

'); - - done(); - }).otherwise(done); - }); - - it("loads templates from themes first", function (done) { - var compileSpy = sandbox.spy(template, 'compileTemplate'), - pathsStub; - - should.exist(template.loadTemplate, 'load template function exists'); - - // In order for the test to work, need to replace the path to the template - pathsStub = sandbox.stub(config, "paths", function () { - return { - // Forcing the theme path to be the same - themePath: path.join(process.cwd(), testTemplatePath), - helperTemplates: path.join(process.cwd(), testTemplatePath) - }; - }); - apiStub = sandbox.stub(api.settings , 'read', function () { - return when({value: 'theme'}); - }); - - template.loadTemplate('test').then(function (templateFn) { - // test that compileTemplate was called with the expected path - compileSpy.calledOnce.should.equal(true); - compileSpy.calledWith(path.join(process.cwd(), testTemplatePath, 'theme', 'partials', 'test.hbs')).should.equal(true); - - should.exist(templateFn); - _.isFunction(templateFn).should.equal(true); - - templateFn().should.equal('

HelloWorld Themed

'); - - compileSpy.restore(); - pathsStub.restore(); + var safeString = template.execute('test', {name: 'world'}); - done(); - }).then(null, done); + should.exist(safeString); + safeString.should.have.property('string').and.equal('

Hello world

'); }); }); \ No newline at end of file diff --git a/core/test/unit/server_helpers_index_spec.js b/core/test/unit/server_helpers_index_spec.js index d98755b6f7d..b21b6386eab 100644 --- a/core/test/unit/server_helpers_index_spec.js +++ b/core/test/unit/server_helpers_index_spec.js @@ -6,9 +6,10 @@ var testUtils = require('../utils'), _ = require('underscore'), path = require('path'), api = require('../../server/api'), + hbs = require('express-hbs'), // Stuff we are testing - handlebars = require('express-hbs').handlebars, + handlebars = hbs.handlebars, helpers = require('../../server/helpers'), config = require('../../server/config'); @@ -19,6 +20,7 @@ describe('Core Helpers', function () { apiStub; beforeEach(function (done) { + var adminHbs = hbs.create(); sandbox = sinon.sandbox.create(); apiStub = sandbox.stub(api.settings , 'read', function () { return when({value: 'casper'}); @@ -34,9 +36,12 @@ describe('Core Helpers', function () { }; }); - helpers.loadCoreHelpers(config).then(function () { + helpers.loadCoreHelpers(config, adminHbs); + // Load template helpers in handlebars + hbs.express3({ partialsDir: [config.paths().helperTemplates] }); + hbs.cachePartials(function(){ done(); - }, done); + }) }); afterEach(function () { @@ -368,101 +373,80 @@ describe('Core Helpers', function () { should.exist(handlebars.helpers.pagination); }); - it('can render single page with no pagination necessary', function (done) { - var rendered; - helpers.loadCoreHelpers().then(function () { - rendered = helpers.pagination.call({pagination: {page: 1, prev: undefined, next: undefined, limit: 15, total: 8, pages: 1}}); - should.exist(rendered); - // strip out carriage returns and compare. - rendered.string.should.match(paginationRegex); - rendered.string.should.match(pageRegex); - rendered.string.should.match(/Page 1 of 1/); - rendered.string.should.not.match(newerRegex); - rendered.string.should.not.match(olderRegex); - done(); - }).then(null, done); + it('can render single page with no pagination necessary', function () { + var rendered = helpers.pagination.call({pagination: {page: 1, prev: undefined, next: undefined, limit: 15, total: 8, pages: 1}}); + should.exist(rendered); + // strip out carriage returns and compare. + rendered.string.should.match(paginationRegex); + rendered.string.should.match(pageRegex); + rendered.string.should.match(/Page 1 of 1/); + rendered.string.should.not.match(newerRegex); + rendered.string.should.not.match(olderRegex); }); - it('can render first page of many with older posts link', function (done) { - var rendered; - helpers.loadCoreHelpers().then(function () { - rendered = helpers.pagination.call({pagination: {page: 1, prev: undefined, next: 2, limit: 15, total: 8, pages: 3}}); - should.exist(rendered); + it('can render first page of many with older posts link', function () { + var rendered = helpers.pagination.call({pagination: {page: 1, prev: undefined, next: 2, limit: 15, total: 8, pages: 3}}); + should.exist(rendered); - rendered.string.should.match(paginationRegex); - rendered.string.should.match(pageRegex); - rendered.string.should.match(olderRegex); - rendered.string.should.match(/Page 1 of 3/); - rendered.string.should.not.match(newerRegex); - done(); - }).then(null, done); + rendered.string.should.match(paginationRegex); + rendered.string.should.match(pageRegex); + rendered.string.should.match(olderRegex); + rendered.string.should.match(/Page 1 of 3/); + rendered.string.should.not.match(newerRegex); }); - it('can render middle pages of many with older and newer posts link', function (done) { - var rendered; - helpers.loadCoreHelpers().then(function () { - rendered = helpers.pagination.call({pagination: {page: 2, prev: 1, next: 3, limit: 15, total: 8, pages: 3}}); - should.exist(rendered); - - rendered.string.should.match(paginationRegex); - rendered.string.should.match(pageRegex); - rendered.string.should.match(olderRegex); - rendered.string.should.match(newerRegex); - rendered.string.should.match(/Page 2 of 3/); + it('can render middle pages of many with older and newer posts link', function () { + var rendered = helpers.pagination.call({pagination: {page: 2, prev: 1, next: 3, limit: 15, total: 8, pages: 3}}); + should.exist(rendered); - done(); - }).then(null, done); + rendered.string.should.match(paginationRegex); + rendered.string.should.match(pageRegex); + rendered.string.should.match(olderRegex); + rendered.string.should.match(newerRegex); + rendered.string.should.match(/Page 2 of 3/); }); - it('can render last page of many with newer posts link', function (done) { - var rendered; - helpers.loadCoreHelpers().then(function () { - rendered = helpers.pagination.call({pagination: {page: 3, prev: 2, next: undefined, limit: 15, total: 8, pages: 3}}); - should.exist(rendered); - - rendered.string.should.match(paginationRegex); - rendered.string.should.match(pageRegex); - rendered.string.should.match(newerRegex); - rendered.string.should.match(/Page 3 of 3/); - rendered.string.should.not.match(olderRegex); + it('can render last page of many with newer posts link', function () { + var rendered = helpers.pagination.call({pagination: {page: 3, prev: 2, next: undefined, limit: 15, total: 8, pages: 3}}); + should.exist(rendered); - done(); - }).then(null, done); + rendered.string.should.match(paginationRegex); + rendered.string.should.match(pageRegex); + rendered.string.should.match(newerRegex); + rendered.string.should.match(/Page 3 of 3/); + rendered.string.should.not.match(olderRegex); }); - it('validates values', function (done) { - helpers.loadCoreHelpers().then(function () { - var runErrorTest = function (data) { - return function () { - helpers.pagination.call(data); - }; + it('validates values', function () { + + var runErrorTest = function (data) { + return function () { + helpers.pagination.call(data); }; + }; - runErrorTest({pagination: {page: 3, prev: true, next: undefined, limit: 15, total: 8, pages: 3}}) - .should.throwError('Invalid value, Next/Prev must be a number'); - runErrorTest({pagination: {page: 3, prev: 2, next: true, limit: 15, total: 8, pages: 3}}) - .should.throwError('Invalid value, Next/Prev must be a number'); - - runErrorTest({pagination: {limit: 15, total: 8, pages: 3}}) - .should.throwError('All values must be defined for page, pages, limit and total'); - runErrorTest({pagination: {page: 3, total: 8, pages: 3}}) - .should.throwError('All values must be defined for page, pages, limit and total'); - runErrorTest({pagination: {page: 3, limit: 15, pages: 3}}) - .should.throwError('All values must be defined for page, pages, limit and total'); - runErrorTest({pagination: {page: 3, limit: 15, total: 8}}) - .should.throwError('All values must be defined for page, pages, limit and total'); - - runErrorTest({pagination: {page: null, limit: 15, total: 8, pages: 3}}) - .should.throwError('Invalid value, check page, pages, limit and total are numbers'); - runErrorTest({pagination: {page: 1, limit: null, total: 8, pages: 3}}) - .should.throwError('Invalid value, check page, pages, limit and total are numbers'); - runErrorTest({pagination: {page: 1, limit: 15, total: null, pages: 3}}) - .should.throwError('Invalid value, check page, pages, limit and total are numbers'); - runErrorTest({pagination: {page: 1, limit: 15, total: 8, pages: null}}) - .should.throwError('Invalid value, check page, pages, limit and total are numbers'); - - done(); - }).then(null, done); + runErrorTest({pagination: {page: 3, prev: true, next: undefined, limit: 15, total: 8, pages: 3}}) + .should.throwError('Invalid value, Next/Prev must be a number'); + runErrorTest({pagination: {page: 3, prev: 2, next: true, limit: 15, total: 8, pages: 3}}) + .should.throwError('Invalid value, Next/Prev must be a number'); + + runErrorTest({pagination: {limit: 15, total: 8, pages: 3}}) + .should.throwError('All values must be defined for page, pages, limit and total'); + runErrorTest({pagination: {page: 3, total: 8, pages: 3}}) + .should.throwError('All values must be defined for page, pages, limit and total'); + runErrorTest({pagination: {page: 3, limit: 15, pages: 3}}) + .should.throwError('All values must be defined for page, pages, limit and total'); + runErrorTest({pagination: {page: 3, limit: 15, total: 8}}) + .should.throwError('All values must be defined for page, pages, limit and total'); + + runErrorTest({pagination: {page: null, limit: 15, total: 8, pages: 3}}) + .should.throwError('Invalid value, check page, pages, limit and total are numbers'); + runErrorTest({pagination: {page: 1, limit: null, total: 8, pages: 3}}) + .should.throwError('Invalid value, check page, pages, limit and total are numbers'); + runErrorTest({pagination: {page: 1, limit: 15, total: null, pages: 3}}) + .should.throwError('Invalid value, check page, pages, limit and total are numbers'); + runErrorTest({pagination: {page: 1, limit: 15, total: 8, pages: null}}) + .should.throwError('Invalid value, check page, pages, limit and total are numbers'); }); }); diff --git a/package.json b/package.json index c73cfab4c71..e37e7b794ab 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "connect-slashes": "1.0.2", "downsize": "0.0.4", "express": "3.4.6", - "express-hbs": "0.5.1", + "express-hbs": "0.5.2", "fs-extra": "0.8.1", "knex": "0.5.0", "moment": "2.4.0",