From 9d7049cd3f07d98b727134d36e3302a7edc9a376 Mon Sep 17 00:00:00 2001
From: Hannah Wolfe <github.erisds@gmail.com>
Date: Mon, 4 Oct 2021 16:50:07 +0100
Subject: [PATCH] Refactored helper registration code into a service

- The helper registration code is "framework" code and very specific
- At the moment the "theme engine" is full of lots of disparate theme related stuff
- I'm trying to make the frontend framework code clearer and also expand it to  make it more useful
- The helper system now also exposes 3 methods allowing you to register a directory, a helper or an alias
- I've updated the codebase to use these both for our core helpers and for "apps"
---
 core/boot.js                                  | 17 +++++-
 core/frontend/apps/amp/index.js               |  8 +--
 core/frontend/apps/amp/lib/helpers/index.js   | 18 -------
 core/frontend/apps/private-blogging/index.js  |  5 +-
 .../private-blogging/lib/helpers/index.js     |  3 --
 core/frontend/helpers/get.js                  |  4 +-
 core/frontend/helpers/ghost_head.js           |  2 +
 core/frontend/helpers/prev_post.js            |  5 +-
 core/frontend/services/apps/proxy.js          |  9 ++--
 .../register.js => helpers/handlebars.js}     |  8 +--
 core/frontend/services/helpers/index.js       | 18 +++++++
 core/frontend/services/helpers/registry.js    | 45 ++++++++++++++++
 core/frontend/services/rendering.js           |  2 +-
 .../theme-engine/handlebars/helpers.js        | 54 -------------------
 .../theme-engine/handlebars/loader.js         | 19 -------
 core/frontend/services/theme-engine/index.js  |  1 -
 core/server/web/site/app.js                   |  5 +-
 test/unit/services/apps/proxy.test.js         | 13 ++---
 .../theme-engine/handlebars/helpers.test.js   |  4 +-
 19 files changed, 116 insertions(+), 124 deletions(-)
 delete mode 100644 core/frontend/apps/amp/lib/helpers/index.js
 delete mode 100644 core/frontend/apps/private-blogging/lib/helpers/index.js
 rename core/frontend/services/{theme-engine/handlebars/register.js => helpers/handlebars.js} (83%)
 create mode 100644 core/frontend/services/helpers/index.js
 create mode 100644 core/frontend/services/helpers/registry.js
 delete mode 100644 core/frontend/services/theme-engine/handlebars/helpers.js
 delete mode 100644 core/frontend/services/theme-engine/handlebars/loader.js

diff --git a/core/boot.js b/core/boot.js
index 5f5b51746c..017a198658 100644
--- a/core/boot.js
+++ b/core/boot.js
@@ -111,10 +111,10 @@ async function initCore({ghostServer, config}) {
 async function initServicesForFrontend() {
     debug('Begin: initServicesForFrontend');
 
-    debug('Begin: Frontend Routing Settings');
+    debug('Begin: Routing Settings');
     const routeSettings = require('./server/services/route-settings');
     await routeSettings.init();
-    debug('End: Frontend Routing Settings');
+    debug('End: Routing Settings');
 
     debug('Begin: Themes');
     const themeService = require('./server/services/themes');
@@ -124,6 +124,18 @@ async function initServicesForFrontend() {
     debug('End: initServicesForFrontend');
 }
 
+/**
+ * Frontend is intended to be just Ghost's frontend
+ */
+async function initFrontend() {
+    debug('Begin: initFrontend');
+
+    const helperService = require('./frontend/services/helpers');
+    await helperService.init();
+
+    debug('End: initFrontend');
+}
+
 /**
  * At the moment we load our express apps all in one go, they require themselves and are co-located
  * What we want is to be able to optionally load various components and mount them
@@ -331,6 +343,7 @@ async function bootGhost() {
         debug('Begin: Load Ghost Services & Apps');
         await initCore({ghostServer, config});
         await initServicesForFrontend();
+        await initFrontend();
         const ghostApp = await initExpressApps();
         await initDynamicRouting();
         await initServices({config});
diff --git a/core/frontend/apps/amp/index.js b/core/frontend/apps/amp/index.js
index f385e38f2c..e4c3c530b7 100644
--- a/core/frontend/apps/amp/index.js
+++ b/core/frontend/apps/amp/index.js
@@ -1,5 +1,6 @@
+const path = require('path');
+
 const router = require('./lib/router');
-const registerHelpers = require('./lib/helpers');
 const urlUtils = require('../../../shared/url-utils');
 
 // Dirty requires
@@ -22,7 +23,8 @@ module.exports = {
         let ampRoute = '*/amp/';
 
         ghost.routeService.registerRouter(ampRoute, ampRouter);
-
-        registerHelpers(ghost);
+        ghost.helperService.registerDir(path.resolve(__dirname, './lib/helpers'));
+        // we use the {{ghost_head}} helper, but call it {{amp_ghost_head}}, so it's consistent
+        ghost.helperService.registerAlias('amp_ghost_head', 'ghost_head');
     }
 };
diff --git a/core/frontend/apps/amp/lib/helpers/index.js b/core/frontend/apps/amp/lib/helpers/index.js
deleted file mode 100644
index f2d24ef879..0000000000
--- a/core/frontend/apps/amp/lib/helpers/index.js
+++ /dev/null
@@ -1,18 +0,0 @@
-// Dirty require!
-const ghostHead = require('../../../../helpers/ghost_head');
-
-function registerAmpHelpers(ghost) {
-    ghost.helpers.registerAsync('amp_content', require('./amp_content'));
-
-    ghost.helpers.register('amp_components', require('./amp_components'));
-
-    ghost.helpers.register('amp_analytics', require('./amp_analytics'));
-
-    // we use the {{ghost_head}} helper, but call it {{amp_ghost_head}}, so it's consistent
-    ghost.helpers.registerAsync('amp_ghost_head', ghostHead);
-
-    // additional injected styles for use inside the single <style amp-custom> tag
-    ghost.helpers.register('amp_style', require('./amp_style'));
-}
-
-module.exports = registerAmpHelpers;
diff --git a/core/frontend/apps/private-blogging/index.js b/core/frontend/apps/private-blogging/index.js
index 43ce108340..12904385d8 100644
--- a/core/frontend/apps/private-blogging/index.js
+++ b/core/frontend/apps/private-blogging/index.js
@@ -1,10 +1,10 @@
+const path = require('path');
 const tpl = require('@tryghost/tpl');
 const logging = require('@tryghost/logging');
 const errors = require('@tryghost/errors');
 const urlUtils = require('../../../shared/url-utils');
 const middleware = require('./lib/middleware');
 const router = require('./lib/router');
-const registerHelpers = require('./lib/helpers');
 
 const messages = {
     urlCannotContainPrivateSubdir: {
@@ -43,8 +43,7 @@ module.exports = {
         checkSubdir();
 
         ghost.routeService.registerRouter(privateRoute, router);
-
-        registerHelpers(ghost);
+        ghost.helperService.registerDir(path.resolve(__dirname, './lib/helpers'));
     },
 
     setupMiddleware: function setupMiddleware(siteApp) {
diff --git a/core/frontend/apps/private-blogging/lib/helpers/index.js b/core/frontend/apps/private-blogging/lib/helpers/index.js
deleted file mode 100644
index 289c12f988..0000000000
--- a/core/frontend/apps/private-blogging/lib/helpers/index.js
+++ /dev/null
@@ -1,3 +0,0 @@
-module.exports = function registerHelpers(ghost) {
-    ghost.helpers.register('input_password', require('./input_password'));
-};
diff --git a/core/frontend/helpers/get.js b/core/frontend/helpers/get.js
index 4ff53d0dc8..e10ae3afdf 100644
--- a/core/frontend/helpers/get.js
+++ b/core/frontend/helpers/get.js
@@ -13,7 +13,7 @@ const Promise = require('bluebird');
 const jsonpath = require('jsonpath');
 
 const messages = {
-    mustBeCalledAsBlock: 'The {{{helperName}}} helper must be called as a block. E.g. {{#{helperName}}}...{{/{helperName}}}',
+    mustBeCalledAsBlock: 'The {\\{{helperName}}} helper must be called as a block. E.g. {{#{helperName}}}...{{/{helperName}}}',
     invalidResource: 'Invalid resource given to get helper'
 };
 
@@ -194,3 +194,5 @@ module.exports = function get(resource, options) {
         }
     });
 };
+
+module.exports.async = true;
diff --git a/core/frontend/helpers/ghost_head.js b/core/frontend/helpers/ghost_head.js
index 9dec088fc3..2dc642006c 100644
--- a/core/frontend/helpers/ghost_head.js
+++ b/core/frontend/helpers/ghost_head.js
@@ -229,3 +229,5 @@ module.exports = function ghost_head(options) { // eslint-disable-line camelcase
             return new SafeString(head.join('\n    ').trim());
         });
 };
+
+module.exports.async = true;
diff --git a/core/frontend/helpers/prev_post.js b/core/frontend/helpers/prev_post.js
index 61008fd335..38a4356f72 100644
--- a/core/frontend/helpers/prev_post.js
+++ b/core/frontend/helpers/prev_post.js
@@ -12,7 +12,7 @@ const Promise = require('bluebird');
 const moment = require('moment');
 
 const messages = {
-    mustBeCalledAsBlock: 'The {{{helperName}}} helper must be called as a block. E.g. {{#{helperName}}}...{{/{helperName}}}'
+    mustBeCalledAsBlock: 'The {\\{{helperName}}} helper must be called as a block. E.g. {{#{helperName}}}...{{/{helperName}}}'
 };
 
 const createFrame = hbs.handlebars.createFrame;
@@ -102,3 +102,6 @@ module.exports = function prevNext(options) {
     // With the guards out of the way, attempt to build the apiOptions, and then fetch the data
     return fetch.call(this, options, data);
 };
+
+module.exports.async = true;
+module.exports.alias = 'next_post';
diff --git a/core/frontend/services/apps/proxy.js b/core/frontend/services/apps/proxy.js
index 099ab78d9d..8ec395e9bc 100644
--- a/core/frontend/services/apps/proxy.js
+++ b/core/frontend/services/apps/proxy.js
@@ -1,13 +1,14 @@
-const helpers = require('../../services/theme-engine/handlebars/register');
+const helperService = require('../../services/helpers');
 const routingService = require('../../services/routing');
 
 module.exports.getInstance = function getInstance() {
     const appRouter = routingService.registry.getRouter('appRouter');
 
     return {
-        helpers: {
-            register: helpers.registerThemeHelper.bind(helpers),
-            registerAsync: helpers.registerAsyncThemeHelper.bind(helpers)
+        helperService: {
+            registerAlias: helperService.registerAlias.bind(helperService),
+            registerHelper: helperService.registerHelper.bind(helperService),
+            registerDir: helperService.registerDir.bind(helperService)
         },
         // Expose the route service...
         routeService: {
diff --git a/core/frontend/services/theme-engine/handlebars/register.js b/core/frontend/services/helpers/handlebars.js
similarity index 83%
rename from core/frontend/services/theme-engine/handlebars/register.js
rename to core/frontend/services/helpers/handlebars.js
index 145cd78f31..a071ec7d75 100644
--- a/core/frontend/services/theme-engine/handlebars/register.js
+++ b/core/frontend/services/helpers/handlebars.js
@@ -1,9 +1,9 @@
 const Promise = require('bluebird');
 const errors = require('@tryghost/errors');
-const hbs = require('../../theme-engine/engine');
-const config = require('../../../../shared/config');
 const logging = require('@tryghost/logging');
 
+const {hbs} = require('../rendering');
+
 // Register an async handlebars helper for a given handlebars instance
 function asyncHelperWrapper(hbsInstance, name, fn) {
     hbsInstance.registerAsyncHelper(name, function returnAsync(context, options, cb) {
@@ -13,7 +13,7 @@ function asyncHelperWrapper(hbsInstance, name, fn) {
             options = undefined;
         }
 
-        // Wrap the function passed in with a when.resolve so it can return either a promise or a value
+        // Wrap the function passed in with a Promise.resolve so it can return either a promise or a value
         Promise.resolve(fn.call(this, context, options)).then(function asyncHelperSuccess(result) {
             cb(result);
         }).catch(function asyncHelperError(err) {
@@ -25,7 +25,7 @@ function asyncHelperWrapper(hbsInstance, name, fn) {
                 }
             });
 
-            const result = config.get('env') === 'development' ? wrappedErr : '';
+            const result = process.env.NODE_ENV === 'development' ? wrappedErr : '';
 
             logging.error(wrappedErr);
 
diff --git a/core/frontend/services/helpers/index.js b/core/frontend/services/helpers/index.js
new file mode 100644
index 0000000000..7355be593e
--- /dev/null
+++ b/core/frontend/services/helpers/index.js
@@ -0,0 +1,18 @@
+const registry = require('./registry');
+
+const path = require('path');
+
+// Initialise Ghost's own helpers
+// This is a weird place for this to live!
+const init = async () => {
+    const helperPath = path.join(__dirname, '../../', 'helpers');
+    return await registry.registerDir(helperPath);
+};
+
+// Oh look! A framework for helpers :D
+module.exports = {
+    registerAlias: registry.registerAlias,
+    registerDir: registry.registerDir,
+    registerHelper: registry.registerHelper,
+    init
+};
diff --git a/core/frontend/services/helpers/registry.js b/core/frontend/services/helpers/registry.js
new file mode 100644
index 0000000000..b91d073e8e
--- /dev/null
+++ b/core/frontend/services/helpers/registry.js
@@ -0,0 +1,45 @@
+const glob = require('glob');
+const path = require('path');
+
+const handlebars = require('./handlebars');
+
+// Internal Cache
+const registry = {};
+
+const registerHelper = (name, helperFn) => {
+    if (registry[name]) {
+        return;
+    }
+
+    registry[name] = helperFn;
+
+    if (helperFn.async) {
+        handlebars.registerAsyncThemeHelper(name, helperFn);
+    } else {
+        handlebars.registerThemeHelper(name, helperFn);
+    }
+};
+
+const registerDir = (helperPath) => {
+    let helperFiles = glob.sync('!(index).js', {cwd: helperPath});
+    helperFiles.forEach((helper) => {
+        const name = helper.replace(/.js$/, '');
+        const fn = require(path.join(helperPath, helper));
+
+        registerHelper(name, fn);
+
+        if (fn.alias) {
+            registerHelper(fn.alias, fn);
+        }
+    });
+};
+
+const registerAlias = (alias, name) => {
+    registerHelper(alias, registry[name]);
+};
+
+module.exports = {
+    registerAlias,
+    registerHelper,
+    registerDir
+};
diff --git a/core/frontend/services/rendering.js b/core/frontend/services/rendering.js
index 28603da290..2ae86a7bd8 100644
--- a/core/frontend/services/rendering.js
+++ b/core/frontend/services/rendering.js
@@ -2,7 +2,7 @@
  * This is a loose concept of a frontend rendering framework
  * Note: everything here gets deep-required from the theme-engine
  * This indicates that the theme engine is a set of services, rather than a single service
- * and could do with a refactor.
+ * and could do with a further refactor.
  *
  * This at least keeps the deep requires in a single place.
  */
diff --git a/core/frontend/services/theme-engine/handlebars/helpers.js b/core/frontend/services/theme-engine/handlebars/helpers.js
deleted file mode 100644
index 7c774e1c3c..0000000000
--- a/core/frontend/services/theme-engine/handlebars/helpers.js
+++ /dev/null
@@ -1,54 +0,0 @@
-const register = require('./register');
-const loader = require('./loader');
-const coreHelpers = loader.getHelpers();
-const registerThemeHelper = register.registerThemeHelper;
-const registerAsyncThemeHelper = register.registerAsyncThemeHelper;
-
-const registerAllCoreHelpers = function registerAllCoreHelpers() {
-    // Register theme helpers
-    registerThemeHelper('asset', coreHelpers.asset);
-    registerThemeHelper('author', coreHelpers.author);
-    registerThemeHelper('authors', coreHelpers.authors);
-    registerThemeHelper('body_class', coreHelpers.body_class);
-    registerThemeHelper('cancel_link', coreHelpers.cancel_link);
-    registerThemeHelper('concat', coreHelpers.concat);
-    registerThemeHelper('content', coreHelpers.content);
-    registerThemeHelper('products', coreHelpers.products);
-    registerThemeHelper('date', coreHelpers.date);
-    registerThemeHelper('encode', coreHelpers.encode);
-    registerThemeHelper('excerpt', coreHelpers.excerpt);
-    registerThemeHelper('foreach', coreHelpers.foreach);
-    registerThemeHelper('ghost_foot', coreHelpers.ghost_foot);
-    registerThemeHelper('has', coreHelpers.has);
-    registerThemeHelper('is', coreHelpers.is);
-    registerThemeHelper('img_url', coreHelpers.img_url);
-    registerThemeHelper('lang', coreHelpers.lang);
-    registerThemeHelper('link', coreHelpers.link);
-    registerThemeHelper('link_class', coreHelpers.link_class);
-    registerThemeHelper('match', coreHelpers.match);
-    registerThemeHelper('meta_description', coreHelpers.meta_description);
-    registerThemeHelper('meta_title', coreHelpers.meta_title);
-    registerThemeHelper('navigation', coreHelpers.navigation);
-    registerThemeHelper('page_url', coreHelpers.page_url);
-    registerThemeHelper('pagination', coreHelpers.pagination);
-    registerThemeHelper('plural', coreHelpers.plural);
-    registerThemeHelper('post_class', coreHelpers.post_class);
-    registerThemeHelper('price', coreHelpers.price);
-    registerThemeHelper('raw', coreHelpers.raw);
-    registerThemeHelper('reading_time', coreHelpers.reading_time);
-    registerThemeHelper('t', coreHelpers.t);
-    registerThemeHelper('tags', coreHelpers.tags);
-    registerThemeHelper('title', coreHelpers.title);
-    registerThemeHelper('twitter_url', coreHelpers.twitter_url);
-    registerThemeHelper('facebook_url', coreHelpers.facebook_url);
-    registerThemeHelper('url', coreHelpers.url);
-
-    // Async theme helpers
-    registerAsyncThemeHelper('ghost_head', coreHelpers.ghost_head);
-    registerAsyncThemeHelper('next_post', coreHelpers.prev_post);
-    registerAsyncThemeHelper('prev_post', coreHelpers.prev_post);
-    registerAsyncThemeHelper('get', coreHelpers.get);
-};
-
-module.exports = coreHelpers;
-module.exports.loadCoreHelpers = registerAllCoreHelpers;
diff --git a/core/frontend/services/theme-engine/handlebars/loader.js b/core/frontend/services/theme-engine/handlebars/loader.js
deleted file mode 100644
index 304b78c6d8..0000000000
--- a/core/frontend/services/theme-engine/handlebars/loader.js
+++ /dev/null
@@ -1,19 +0,0 @@
-const glob = require('glob');
-const path = require('path');
-
-const helperPath = path.join(__dirname, '../../../', 'helpers');
-
-module.exports.getHelpers = () => {
-    const helpers = {};
-
-    // We use glob here because it's already a dependency
-    // If we want to get rid of glob we could use E.g. requiredir
-    // Or require('fs').readdirSync(__dirname + '/')
-    let helperFiles = glob.sync('!(index).js', {cwd: helperPath});
-    helperFiles.forEach((helper) => {
-        let name = helper.replace(/.js$/, '');
-        helpers[name] = require(path.join(helperPath, helper));
-    });
-
-    return helpers;
-};
diff --git a/core/frontend/services/theme-engine/index.js b/core/frontend/services/theme-engine/index.js
index 85f6b05bdc..669c48b1b1 100644
--- a/core/frontend/services/theme-engine/index.js
+++ b/core/frontend/services/theme-engine/index.js
@@ -3,6 +3,5 @@ const active = require('./active');
 module.exports = {
     getActive: active.get,
     setActive: active.set,
-    loadCoreHelpers: require('./handlebars/helpers').loadCoreHelpers,
     middleware: require('./middleware')
 };
diff --git a/core/server/web/site/app.js b/core/server/web/site/app.js
index 4e1f987edf..a15d79a843 100644
--- a/core/server/web/site/app.js
+++ b/core/server/web/site/app.js
@@ -114,8 +114,9 @@ module.exports = function setupSiteApp(options = {}) {
     // 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
-    themeEngine.loadCoreHelpers();
-    debug('Helpers done');
+    // themeEngine.loadCoreHelpers();
+    // themeEngine.registerHandlebarsHelpers();
+    // debug('Helpers done');
 
     // Global handling for member session, ensures a member is logged in to the frontend
     siteApp.use(membersService.middleware.loadMemberSession);
diff --git a/test/unit/services/apps/proxy.test.js b/test/unit/services/apps/proxy.test.js
index 26b08fba28..b92af527b4 100644
--- a/test/unit/services/apps/proxy.test.js
+++ b/test/unit/services/apps/proxy.test.js
@@ -1,6 +1,6 @@
 const should = require('should');
 const sinon = require('sinon');
-const helpers = require('../../../../core/frontend/services/theme-engine/handlebars/register');
+const helpers = require('../../../../core/frontend/services/helpers');
 const AppProxy = require('../../../../core/frontend/services/apps/proxy');
 const routing = require('../../../../core/frontend/services/routing');
 
@@ -19,16 +19,17 @@ describe('Apps', function () {
         it('creates a ghost proxy', function () {
             const appProxy = AppProxy.getInstance('TestApp');
 
-            should.exist(appProxy.helpers);
-            should.exist(appProxy.helpers.register);
-            should.exist(appProxy.helpers.registerAsync);
+            should.exist(appProxy.helperService);
+            should.exist(appProxy.helperService.registerAlias);
+            should.exist(appProxy.helperService.registerDir);
+            should.exist(appProxy.helperService.registerHelper);
         });
 
         it('allows helper registration', function () {
-            const registerSpy = sinon.stub(helpers, 'registerThemeHelper');
+            const registerSpy = sinon.stub(helpers, 'registerHelper');
             const appProxy = AppProxy.getInstance('TestApp');
 
-            appProxy.helpers.register('myTestHelper', sinon.stub().returns('test result'));
+            appProxy.helperService.registerHelper('myTestHelper', sinon.stub().returns('test result'));
 
             registerSpy.called.should.equal(true);
         });
diff --git a/test/unit/services/theme-engine/handlebars/helpers.test.js b/test/unit/services/theme-engine/handlebars/helpers.test.js
index 1d5f823139..1ec00390be 100644
--- a/test/unit/services/theme-engine/handlebars/helpers.test.js
+++ b/test/unit/services/theme-engine/handlebars/helpers.test.js
@@ -3,7 +3,7 @@ const _ = require('lodash');
 const hbs = require('../../../../../core/frontend/services/theme-engine/engine');
 
 // Stuff we are testing
-const helpers = require('../../../../../core/frontend/services/theme-engine/handlebars/helpers');
+const helpers = require('../../../../../core/frontend/services/helpers');
 
 describe('Helpers', function () {
     const hbsHelpers = ['each', 'if', 'unless', 'with', 'helperMissing', 'blockHelperMissing', 'log', 'lookup', 'block', 'contentFor'];
@@ -20,7 +20,7 @@ describe('Helpers', function () {
     describe('Load Core Helpers', function () {
         before(function () {
             hbs.express4();
-            helpers.loadCoreHelpers();
+            helpers.init();
         });
 
         // This will work when we finish refactoring