🔥 🎨 Cleanup & simplify theme helpers (#8223)

no issue 🔥 Remove adminHbs concept from tests 🔥 Get rid of unnecessary helper test utils 🔥 Remove helper missing code - this hasn't been registered / used for ages 😱 - gscan no longer allows us to activate themes that have missing helpers, so this wouldn't be used anyway TODO: consider whether we should make a way to override this? 🎨 Reduce coupling inside of /helpers 🎨 Use settingsCache in ghost_foot ✨ Labs util for enabling helpers 🎨 Move loadCoreHelpers to blog - This needs a proper home, but at the very least it doesn't belong in server/app.js! 🎨 Use settingsCache in ghost_head
TryGhost · Mar 23, 2017 · 3cea203 · 3cea203
1 parent 5b161a2
commit 3cea203
Show file tree

Hide file tree

Showing 40 changed files with 230 additions and 479 deletions.
diff --git a/core/server/app.js b/core/server/app.js
@@ -52,11 +52,6 @@ module.exports = function setupParentApp() {
     // This sets global res.locals which are needed everywhere
     parentApp.use(ghostLocals);
 
-    // @TODO where should this live?!
-    // Load helpers
-    require('./helpers').loadCoreHelpers();
-    debug('Helpers done');
-
     // Mount the  apps on the parentApp
     // API
     // @TODO: finish refactoring the API app

diff --git a/core/server/apps/subscribers/lib/helpers/index.js b/core/server/apps/subscribers/lib/helpers/index.js
@@ -1,32 +1,20 @@
 // Dirty requires!
-var hbs = require('express-hbs'),
-    logging = require('../../../../logging'),
-    i18n = require('../../../../i18n'),
-    labs = require('../../../../utils/labs'),
-
-    errorMessages = [
-        i18n.t('warnings.helpers.helperNotAvailable', {helperName: 'subscribe_form'}),
-        i18n.t('warnings.helpers.apiMustBeEnabled', {helperName: 'subscribe_form', flagName: 'subscribers'}),
-        i18n.t('warnings.helpers.seeLink', {url: 'http://support.ghost.org/subscribers-beta/'})
-    ];
+var labs = require('../../../../utils/labs');
 
 module.exports = function registerHelpers(ghost) {
-    var err;
-
     ghost.helpers.register('input_email', require('./input_email'));
 
     ghost.helpers.register('subscribe_form', function labsEnabledHelper() {
-        if (labs.isSet('subscribers') === true) {
-            return require('./subscribe_form').apply(this, arguments);
-        }
-
-        err = new Error();
-        err.message = i18n.t('warnings.helpers.helperNotAvailable', {helperName: 'subscribe_form'});
-        err.context = i18n.t('warnings.helpers.apiMustBeEnabled', {helperName: 'subscribe_form', flagName: 'subscribers'});
-        err.help = i18n.t('warnings.helpers.seeLink', {url: 'http://support.ghost.org/subscribers-beta/'});
-
-        logging.error(err);
+        var self = this,
+            args = arguments;
 
-        return new hbs.handlebars.SafeString('<script>console.error("' + errorMessages.join(' ') + '");</script>');
+        return labs.enabledHelper({
+            flagKey: 'subscribers',
+            flagName: 'Subscribers',
+            helperName: 'subscribe_form',
+            helpUrl: 'http://support.ghost.org/subscribers-beta/'
+        }, function executeHelper() {
+            return require('./subscribe_form').apply(self, args);
+        });
     });
 };
diff --git a/core/server/blog/app.js b/core/server/blog/app.js
@@ -53,6 +53,13 @@ module.exports = function setupBlogApp() {
     // Serve blog images using the storage adapter
     blogApp.use('/' + utils.url.STATIC_IMAGE_URL_PREFIX, storage.getStorage().serve());
 
+    // @TODO find this a better home
+    // We do this here, at the top level, because helpers require so much stuff.
+    // Moving this to being inside themes, where it probably should be requires the proxy to be refactored
+    // Else we end up with circular dependencies
+    require('../helpers').loadCoreHelpers();
+    debug('Helpers done');
+
     // 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

diff --git a/core/server/helpers/get.js b/core/server/helpers/get.js
@@ -1,24 +1,24 @@
 // # Get Helper
 // Usage: `{{#get "posts" limit="5"}}`, `{{#get "tags" limit="all"}}`
 // Fetches data from the API
-var _               = require('lodash'),
-    hbs             = require('express-hbs'),
-    Promise         = require('bluebird'),
-    errors          = require('../errors'),
-    logging         = require('../logging'),
-    api             = require('../api'),
-    jsonpath        = require('jsonpath'),
-    labs            = require('../utils/labs'),
-    i18n            = require('../i18n'),
+var _ = require('lodash'),
+    hbs = require('express-hbs'),
+    Promise = require('bluebird'),
+    jsonpath = require('jsonpath'),
+
+    logging = require('../logging'),
+    i18n = require('../i18n'),
+    api = require('../api'),
+    labs = require('../utils/labs'),
     resources,
     pathAliases,
     get;
 
 // Endpoints that the helper is able to access
-resources =  ['posts', 'tags', 'users'];
+resources = ['posts', 'tags', 'users'];
 
 // Short forms of paths which we should understand
-pathAliases     = {
+pathAliases = {
     'post.tags': 'post.tags[*].slug',
     'post.author': 'post.author.slug'
 };
@@ -139,23 +139,17 @@ get = function get(resource, options) {
     });
 };
 
-module.exports = function getWithLabs(resource, options) {
-    var self = this, err;
-
-    if (labs.isSet('publicAPI') === true) {
-        // get helper is  active
-        return get.call(self, resource, options);
-    } else {
-        err = new errors.GhostError({
-            message: i18n.t('warnings.helpers.get.helperNotAvailable'),
-            context: i18n.t('warnings.helpers.get.apiMustBeEnabled'),
-            help: i18n.t('warnings.helpers.get.seeLink', {url: 'http://support.ghost.org/public-api-beta'})
-        });
-
-        logging.error(err);
-
-        return Promise.resolve(function noGetHelper() {
-            return '<script>console.error(' + JSON.stringify(err) + ');</script>';
-        });
-    }
+module.exports = function getLabsWrapper() {
+    var self = this,
+        args = arguments;
+
+    return labs.enabledHelper({
+        flag: 'publicAPI',
+        flagName: 'Public API',
+        helperName: 'get',
+        helpUrl: 'http://support.ghost.org/public-api-beta/',
+        async: true
+    }, function executeHelper() {
+        return get.apply(self, args);
+    });
 };
diff --git a/core/server/helpers/ghost_foot.js b/core/server/helpers/ghost_foot.js
@@ -5,24 +5,26 @@
 //
 // We use the name ghost_foot to match the helper for consistency:
 // jscs:disable requireCamelCaseOrUpperCaseIdentifiers
-
-var hbs             = require('express-hbs'),
-    _               = require('lodash'),
-    filters         = require('../filters'),
-    api             = require('../api'),
+var hbs = require('express-hbs'),
+    SafeString = hbs.handlebars.SafeString,
+    _ = require('lodash'),
+    filters = require('../filters'),
+    settingsCache = require('../settings/cache'),
     ghost_foot;
 
-ghost_foot = function (options) {
-    /*jshint unused:false*/
-    var foot = [];
+ghost_foot = function ghost_foot() {
+    var foot = [],
+        codeInjection = settingsCache.get('ghost_foot');
+
+    if (!_.isEmpty(codeInjection)) {
+        foot.push(codeInjection);
+    }
 
-    return api.settings.read({key: 'ghost_foot'}).then(function (response) {
-        foot.push(response.settings[0].value);
-        return filters.doFilter('ghost_foot', foot);
-    }).then(function (foot) {
-        var footString = _.reduce(foot, function (memo, item) { return memo + ' ' + item; }, '');
-        return new hbs.handlebars.SafeString(footString.trim());
-    });
+    return filters
+        .doFilter('ghost_foot', foot)
+        .then(function (foot) {
+            return new SafeString(foot.join(' ').trim());
+        });
 };
 
 module.exports = ghost_foot;
diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js
@@ -85,6 +85,7 @@ function ghost_head(options) {
     var metaData,
         client,
         head = [],
+        codeInjection = settingsCache.get('ghost_head'),
         context = this.context ? this.context : null,
         useStructuredData = !config.isPrivacyDisabled('useStructuredData'),
         safeVersion = this.safeVersion,
@@ -153,11 +154,9 @@ function ghost_head(options) {
             escapeExpression(metaData.blog.title)  + '" href="' +
             escapeExpression(metaData.rssUrl) + '" />');
 
-        return api.settings.read({key: 'ghost_head'});
-    }).then(function (response) {
         // no code injection for amp context!!!
-        if (!_.includes(context, 'amp')) {
-            head.push(response.settings[0].value);
+        if (!_.includes(context, 'amp') && !_.isEmpty(codeInjection)) {
+            head.push(codeInjection);
         }
         return filters.doFilter('ghost_head', head);
     }).then(function (head) {

diff --git a/core/server/helpers/index.js b/core/server/helpers/index.js
@@ -1,13 +1,12 @@
 var hbs = require('express-hbs'),
     Promise = require('bluebird'),
     errors = require('../errors'),
-    logging = require('../logging'),
-    utils = require('./utils'),
-    i18n = require('../i18n'),
+    config = require('../config'),
     coreHelpers = {},
     registerHelpers;
 
-if (!utils.isProduction) {
+// @TODO think about a config option for this e.g. theme.devmode?
+if (config.get('env') !== 'production') {
     hbs.handlebars.logger.level = 0;
 }
 
@@ -40,16 +39,6 @@ coreHelpers.title = require('./title');
 coreHelpers.twitter_url = require('./twitter_url');
 coreHelpers.url = require('./url');
 
-coreHelpers.helperMissing = function (arg) {
-    if (arguments.length === 2) {
-        return undefined;
-    }
-
-    logging.error(new errors.GhostError({
-        message: i18n.t('warnings.helpers.index.missingHelper', {arg: arg})
-    }));
-};
-
 // Register an async handlebars helper for a given handlebars instance
 function registerAsyncHelper(hbs, name, fn) {
     hbs.registerAsyncHelper(name, function (context, options, cb) {

diff --git a/core/server/helpers/utils.js b/core/server/helpers/utils.js
@@ -1,13 +1,11 @@
 var _       = require('lodash'),
-    config  = require('../config'),
     utils;
 
 utils = {
     assetTemplate: _.template('<%= source %>?v=<%= version %>'),
     linkTemplate: _.template('<a href="<%= url %>"><%= text %></a>'),
     scriptTemplate: _.template('<script src="<%= source %>?v=<%= version %>"></script>'),
     inputTemplate: _.template('<input class="<%= className %>" type="<%= type %>" name="<%= name %>" <%= extras %> />'),
-    isProduction: config.get('env') === 'production',
     // @TODO this can probably be made more generic and used in more places
     findKey: function findKey(key, object, data) {
         if (object && _.has(object, key) && !_.isEmpty(object[key])) {

diff --git a/core/server/translations/en.json b/core/server/translations/en.json
@@ -483,17 +483,14 @@
         },
         "helpers": {
             "helperNotAvailable": "The \\{\\{{helperName}\\}\\} helper is not available.",
-            "apiMustBeEnabled": "The {flagName} labs flag must be enabled if you wish to use the \\{\\{{helperName}\\}\\} helper.",
+            "flagMustBeEnabled": "The {flagName} labs flag must be enabled if you wish to use the \\{\\{{helperName}\\}\\} helper.",
             "seeLink": "See {url}",
             "foreach": {
                 "iteratorNeeded": "Need to pass an iterator to #foreach"
             },
             "get": {
                 "mustBeCalledAsBlock": "Get helper must be called as a block",
-                "invalidResource": "Invalid resource given to get helper",
-                "helperNotAvailable": "The \\{\\{get\\}\\} helper is not available.",
-                "apiMustBeEnabled": "Public API access must be enabled if you wish to use the \\{\\{get\\}\\} helper.",
-                "seeLink": "See {url}"
+                "invalidResource": "Invalid resource given to get helper"
             },
             "has": {
                 "invalidAttribute": "Invalid or no attribute given to has helper"

diff --git a/core/server/utils/labs.js b/core/server/utils/labs.js
@@ -1,9 +1,46 @@
 var settingsCache = require('../settings/cache'),
-    flagIsSet;
+    _ = require('lodash'),
+    Promise = require('bluebird'),
+    hbs = require('express-hbs'),
+    errors = require('../errors'),
+    logging = require('../logging'),
+    i18n = require('../i18n'),
+    labs = module.exports = {};
 
-flagIsSet = function flagIsSet(flag) {
+labs.isSet = function isSet(flag) {
     var labsConfig = settingsCache.get('labs');
     return labsConfig && labsConfig[flag] && labsConfig[flag] === true;
 };
 
-module.exports.isSet = flagIsSet;
+labs.enabledHelper = function enabledHelper(options, callback) {
+    var errDetails, errString;
+
+    if (labs.isSet(options.flagKey) === true) {
+        // helper is active, use the callback
+        return callback();
+    }
+
+    // Else, the helper is not active and we need to handle this as an error
+    errDetails = {
+        message: i18n.t('warnings.helpers.helperNotAvailable', {helperName: options.helperName}),
+        context: i18n.t('warnings.helpers.flagMustBeEnabled', {
+            helperName: options.helperName,
+            flagName: options.flagName
+        }),
+        help: i18n.t('warnings.helpers.seeLink', {url: options.helpUrl})
+    };
+
+    logging.error(new errors.GhostError(errDetails));
+
+    errString = new hbs.handlebars.SafeString(
+        '<script>console.error("' + _.values(errDetails).join(' ') + '");</script>'
+    );
+
+    if (options.async) {
+        return Promise.resolve(function asyncError() {
+            return errString;
+        });
+    }
+
+    return errString;
+};
diff --git a/core/test/unit/server_helpers/asset_spec.js b/core/test/unit/server_helpers/asset_spec.js
@@ -1,19 +1,15 @@
-var should = require('should'),
+var should = require('should'), // jshint ignore:line
     sinon = require('sinon'),
-    hbs = require('express-hbs'),
-    utils = require('./utils'),
     configUtils = require('../../utils/configUtils'),
     helpers = require('../../../server/helpers'),
     settingsCache = require('../../../server/settings/cache'),
-    handlebars = hbs.handlebars,
 
     sandbox = sinon.sandbox.create();
 
 describe('{{asset}} helper', function () {
     var rendered, localSettingsCache = {};
 
     before(function () {
-        utils.loadHelpers();
         configUtils.set({assetHash: 'abc'});
 
         sandbox.stub(settingsCache, 'get', function (key) {
@@ -26,10 +22,6 @@ describe('{{asset}} helper', function () {
         sandbox.restore();
     });
 
-    it('has loaded asset helper', function () {
-        should.exist(handlebars.helpers.asset);
-    });
-
     describe('no subdirectory', function () {
         it('handles favicon correctly', function () {
             // with ghost set

diff --git a/core/test/unit/server_helpers/author_spec.js b/core/test/unit/server_helpers/author_spec.js
@@ -1,20 +1,9 @@
-var should = require('should'),
-    hbs = require('express-hbs'),
-    utils = require('./utils'),
+var should = require('should'), // jshint ignore:line
 
 // Stuff we are testing
-    handlebars = hbs.handlebars,
     helpers = require('../../../server/helpers');
 
 describe('{{author}} helper', function () {
-    before(function () {
-        utils.loadHelpers();
-    });
-
-    it('has loaded author helper', function () {
-        should.exist(handlebars.helpers.author);
-    });
-
     it('Returns the link to the author from the context', function () {
         var data = {author: {name: 'abc 123', slug: 'abc123', bio: '', website: '', status: '', location: ''}},
             result = helpers.author.call(data, {hash: {}});