From 412d290a307c6a07b544957cb612adfffaeada82 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 11 May 2022 09:51:38 +0100 Subject: [PATCH] Refactored async helpers to use async/await (#14781) - async/await makes the code easier to read - also means we can get rid of usages of Promise, and some unnecessary usages of Bluebird - this refactor is done for the 4 async helpers and the code that wraps async helpers so the whole pipeline is async/await and more understandable --- .../apps/amp/lib/helpers/amp_content.js | 22 +- core/frontend/helpers/get.js | 41 ++-- core/frontend/helpers/ghost_head.js | 222 +++++++++--------- core/frontend/helpers/prev_post.js | 39 ++- core/frontend/services/helpers/handlebars.js | 23 +- 5 files changed, 168 insertions(+), 179 deletions(-) diff --git a/core/frontend/apps/amp/lib/helpers/amp_content.js b/core/frontend/apps/amp/lib/helpers/amp_content.js index 7814a89ae1..ff66f26f96 100644 --- a/core/frontend/apps/amp/lib/helpers/amp_content.js +++ b/core/frontend/apps/amp/lib/helpers/amp_content.js @@ -6,8 +6,6 @@ // // Converts normal HTML into AMP HTML with Amperize module and uses a cache to return it from // there if available. The cacheId is a combination of `updated_at` and the `slug`. -const Promise = require('bluebird'); - const {DateTime, Interval} = require('luxon'); const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); @@ -170,19 +168,16 @@ function getAmperizeHTML(html, post) { return Promise.resolve(amperizeCache[post.id].amp); } -function ampContent() { +module.exports = async function amp_content() { // eslint-disable-line camelcase let sanitizeHtml = require('sanitize-html'); let cheerio = require('cheerio'); - let amperizeHTML = { - amperize: getAmperizeHTML(this.html, this) - }; - - return Promise.props(amperizeHTML).then((result) => { + try { + const response = await getAmperizeHTML(this.html, this); let $ = null; // our Amperized HTML - ampHTML = result.amperize || ''; + ampHTML = response ?? ''; // Use cheerio to traverse through HTML and make little clean-ups $ = cheerio.load(ampHTML); @@ -208,9 +203,12 @@ function ampContent() { }); return new SafeString(cleanHTML); - }); -} + } catch (error) { + logging.error(error); -module.exports = ampContent; + // Return an empty safe string + return new SafeString(''); + } +}; module.exports.async = true; diff --git a/core/frontend/helpers/get.js b/core/frontend/helpers/get.js index 6de324279c..d984795ed9 100644 --- a/core/frontend/helpers/get.js +++ b/core/frontend/helpers/get.js @@ -9,7 +9,6 @@ const errors = require('@tryghost/errors'); const tpl = require('@tryghost/tpl'); const _ = require('lodash'); -const Promise = require('bluebird'); const jsonpath = require('jsonpath'); const messages = { @@ -46,7 +45,6 @@ const pathAliases = { /** * ## Is Browse * Is this a Browse request or a Read request? - * @param {Object} resource * @param {Object} options * @returns {boolean} */ @@ -123,9 +121,9 @@ function parseOptions(globals, data, options) { * ## Get * @param {Object} resource * @param {Object} options - * @returns {Promise} + * @returns {Promise} */ -module.exports = function get(resource, options) { +module.exports = async function get(resource, options) { options = options || {}; options.hash = options.hash || {}; options.data = options.data || {}; @@ -141,13 +139,13 @@ module.exports = function get(resource, options) { if (!options.fn) { data.error = tpl(messages.mustBeCalledAsBlock, {helperName: 'get'}); logging.warn(data.error); - return Promise.resolve(); + return; } if (!RESOURCES[resource]) { data.error = tpl(messages.invalidResource); logging.warn(data.error); - return Promise.resolve(options.inverse(self, {data: data})); + return options.inverse(self, {data: data}); } const controllerName = RESOURCES[resource].alias; @@ -158,34 +156,35 @@ module.exports = function get(resource, options) { apiOptions = parseOptions(ghostGlobals, this, apiOptions); apiOptions.context = {member: data.member}; - // @TODO: https://github.com/TryGhost/Ghost/issues/10548 - return controller[action](apiOptions).then(function success(result) { + try { + const response = await controller[action](apiOptions); + // prepare data properties for use with handlebars - if (result[resource] && result[resource].length) { - result[resource].forEach(prepareContextResource); + if (response[resource] && response[resource].length) { + response[resource].forEach(prepareContextResource); } // used for logging details of slow requests - returnedRowsCount = result[resource] && result[resource].length; + returnedRowsCount = response[resource] && response[resource].length; // block params allows the theme developer to name the data using something like // `{{#get "posts" as |result pageInfo|}}` - const blockParams = [result[resource]]; - if (result.meta && result.meta.pagination) { - result.pagination = result.meta.pagination; - blockParams.push(result.meta.pagination); + const blockParams = [response[resource]]; + if (response.meta && response.meta.pagination) { + response.pagination = response.meta.pagination; + blockParams.push(response.meta.pagination); } // Call the main template function - return options.fn(result, { + return options.fn(response, { data: data, blockParams: blockParams }); - }).catch(function error(err) { - logging.error(err); - data.error = err.message; + } catch (error) { + logging.error(error); + data.error = error.message; return options.inverse(self, {data: data}); - }).finally(function () { + } finally { const totalMs = Date.now() - start; const logLevel = config.get('logging:slowHelper:level'); const threshold = config.get('logging:slowHelper:threshold'); @@ -200,7 +199,7 @@ 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 d963c05a08..6af864b3d0 100644 --- a/core/frontend/helpers/ghost_head.js +++ b/core/frontend/helpers/ghost_head.js @@ -96,7 +96,7 @@ function getMembersHelper(data) { * Also see how the root object gets created, https://github.com/wycats/handlebars.js/blob/v4.0.6/lib/handlebars/runtime.js#L259 */ // We use the name ghost_head to match the helper for consistency: -module.exports = function ghost_head(options) { // eslint-disable-line camelcase +module.exports = async function ghost_head(options) { // eslint-disable-line camelcase debug('begin'); // if server error page do nothing @@ -118,128 +118,126 @@ module.exports = function ghost_head(options) { // eslint-disable-line camelcase debug('preparation complete, begin fetch'); - /** - * @TODO: - * - getMetaData(dataRoot, dataRoot) -> yes that looks confusing! - * - there is a very mixed usage of `data.context` vs. `root.context` vs `root._locals.context` vs. `this.context` - * - NOTE: getMetaData won't live here anymore soon, see https://github.com/TryGhost/Ghost/issues/8995 - * - therefore we get rid of using `getMetaData(this, dataRoot)` - * - dataRoot has access to *ALL* locals, see function description - * - it should not break anything - */ - return getMetaData(dataRoot, dataRoot) - .then(function handleMetaData(meta) { - debug('end fetch'); + try { + /** + * @TODO: + * - getMetaData(dataRoot, dataRoot) -> yes that looks confusing! + * - there is a very mixed usage of `data.context` vs. `root.context` vs `root._locals.context` vs. `this.context` + * - NOTE: getMetaData won't live here anymore soon, see https://github.com/TryGhost/Ghost/issues/8995 + * - therefore we get rid of using `getMetaData(this, dataRoot)` + * - dataRoot has access to *ALL* locals, see function description + * - it should not break anything + */ + const meta = await getMetaData(dataRoot, dataRoot); + debug('end fetch'); - if (context) { - // head is our main array that holds our meta data - if (meta.metaDescription && meta.metaDescription.length > 0) { - head.push(''); - } - - // no output in head if a publication icon is not set - if (settingsCache.get('icon')) { - head.push(''); - } - - head.push(''); - head.push(''); - - // don't allow indexing of preview URLs! - if (_.includes(context, 'preview')) { - head.push(writeMetaTag('robots', 'noindex,nofollow', 'name')); - } - - // show amp link in post when 1. we are not on the amp page and 2. amp is enabled - if (_.includes(context, 'post') && !_.includes(context, 'amp') && settingsCache.get('amp')) { - head.push(''); - } - - if (meta.previousUrl) { - head.push(''); - } - - if (meta.nextUrl) { - head.push(''); - } - - if (!_.includes(context, 'paged') && useStructuredData) { - head.push(''); - head.push.apply(head, finaliseStructuredData(meta)); - head.push(''); - - if (meta.schema) { - head.push('\n'); - } - } + if (context) { + // head is our main array that holds our meta data + if (meta.metaDescription && meta.metaDescription.length > 0) { + head.push(''); } - head.push(''); - - // Ghost analytics tag - if (labs.isSet('membersActivity')) { - const postId = (dataRoot && dataRoot.post) ? dataRoot.post.id : ''; - head.push(writeMetaTag('ghost-analytics-id', postId, 'name')); + // no output in head if a publication icon is not set + if (settingsCache.get('icon')) { + head.push(''); } - head.push(''); + head.push(''); + head.push(''); - // no code injection for amp context!!! - if (!_.includes(context, 'amp')) { - head.push(getMembersHelper(options.data)); - - // @TODO do this in a more "frameworky" way - if (cardAssetService.hasFile('js')) { - head.push(``); - } - if (cardAssetService.hasFile('css')) { - head.push(``); - } - - if (!_.isEmpty(globalCodeinjection)) { - head.push(globalCodeinjection); - } - - if (!_.isEmpty(postCodeInjection)) { - head.push(postCodeInjection); - } - - if (!_.isEmpty(tagCodeInjection)) { - head.push(tagCodeInjection); - } + // don't allow indexing of preview URLs! + if (_.includes(context, 'preview')) { + head.push(writeMetaTag('robots', 'noindex,nofollow', 'name')); } - // AMP template has style injected directly because there can only be one `; - const existingScriptIndex = _.findLastIndex(head, str => str.match(/<\/(style|script)>/)); - - if (existingScriptIndex !== -1) { - head[existingScriptIndex] = head[existingScriptIndex] + styleTag; - } else { - head.push(styleTag); - } + // show amp link in post when 1. we are not on the amp page and 2. amp is enabled + if (_.includes(context, 'post') && !_.includes(context, 'amp') && settingsCache.get('amp')) { + head.push(''); } - debug('end'); - return new SafeString(head.join('\n ').trim()); - }) - .catch(function handleError(err) { - logging.error(err); + if (meta.previousUrl) { + head.push(''); + } - // Return what we have so far (currently nothing) - return new SafeString(head.join('\n ').trim()); - }); + if (meta.nextUrl) { + head.push(''); + } + + if (!_.includes(context, 'paged') && useStructuredData) { + head.push(''); + head.push.apply(head, finaliseStructuredData(meta)); + head.push(''); + + if (meta.schema) { + head.push('\n'); + } + } + } + + head.push(''); + + // Ghost analytics tag + if (labs.isSet('membersActivity')) { + const postId = (dataRoot && dataRoot.post) ? dataRoot.post.id : ''; + head.push(writeMetaTag('ghost-analytics-id', postId, 'name')); + } + + head.push(''); + + // no code injection for amp context!!! + if (!_.includes(context, 'amp')) { + head.push(getMembersHelper(options.data)); + + // @TODO do this in a more "frameworky" way + if (cardAssetService.hasFile('js')) { + head.push(``); + } + if (cardAssetService.hasFile('css')) { + head.push(``); + } + + if (!_.isEmpty(globalCodeinjection)) { + head.push(globalCodeinjection); + } + + if (!_.isEmpty(postCodeInjection)) { + head.push(postCodeInjection); + } + + if (!_.isEmpty(tagCodeInjection)) { + head.push(tagCodeInjection); + } + } + + // AMP template has style injected directly because there can only be one `; + const existingScriptIndex = _.findLastIndex(head, str => str.match(/<\/(style|script)>/)); + + if (existingScriptIndex !== -1) { + head[existingScriptIndex] = head[existingScriptIndex] + styleTag; + } else { + head.push(styleTag); + } + } + + debug('end'); + return new SafeString(head.join('\n ').trim()); + } catch (error) { + logging.error(error); + + // Return what we have so far (currently nothing) + 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 a8170fb1ca..ffa85521b7 100644 --- a/core/frontend/helpers/prev_post.js +++ b/core/frontend/helpers/prev_post.js @@ -9,7 +9,6 @@ const {checks} = require('../services/data'); const logging = require('@tryghost/logging'); const tpl = require('@tryghost/tpl'); const get = require('lodash/get'); -const Promise = require('bluebird'); const moment = require('moment'); const messages = { @@ -55,29 +54,25 @@ const buildApiOptions = function buildApiOptions(options, post) { * @param {*} data * @returns {Promise} */ -const fetch = function fetch(options, data) { +const fetch = async function fetch(options, data) { const self = this; const apiOptions = buildApiOptions(options, this); - // @TODO: https://github.com/TryGhost/Ghost/issues/10548 - const controller = api.postsPublic || api.posts; + try { + const response = await api.postsPublic.browse(apiOptions); - return controller - .browse(apiOptions) - .then(function handleSuccess(result) { - const related = result.posts[0]; + const related = response.posts[0]; - if (related) { - return options.fn(related, {data: data}); - } else { - return options.inverse(self, {data: data}); - } - }) - .catch(function handleError(err) { - logging.error(err); - data.error = err.message; + if (related) { + return options.fn(related, {data: data}); + } else { return options.inverse(self, {data: data}); - }); + } + } catch (error) { + logging.error(error); + data.error = error.message; + return options.inverse(self, {data: data}); + } }; // If prevNext method is called without valid post data then we must return a promise, if there is valid post data @@ -87,7 +82,7 @@ const fetch = function fetch(options, data) { * @param {*} options * @returns {Promise} */ -module.exports = function prevNext(options) { +module.exports = async function prevNext(options) { options = options || {}; const data = createFrame(options.data); @@ -97,16 +92,16 @@ module.exports = function prevNext(options) { if (!options.fn || !options.inverse) { data.error = tpl(messages.mustBeCalledAsBlock, {helperName: options.name}); logging.warn(data.error); - return Promise.resolve(); + return; } if (context.includes('preview')) { - return Promise.resolve(options.inverse(this, {data: data})); + return options.inverse(this, {data: data}); } // Guard against trying to execute prev/next on pages, or other resources if (!checks.isPost(this) || this.page) { - return Promise.resolve(options.inverse(this, {data: data})); + return options.inverse(this, {data: data}); } // With the guards out of the way, attempt to build the apiOptions, and then fetch the data diff --git a/core/frontend/services/helpers/handlebars.js b/core/frontend/services/helpers/handlebars.js index 1b3b18fe76..e56b89a959 100644 --- a/core/frontend/services/helpers/handlebars.js +++ b/core/frontend/services/helpers/handlebars.js @@ -1,4 +1,3 @@ -const Promise = require('bluebird'); const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); @@ -6,31 +5,31 @@ const {hbs} = require('../handlebars'); // Register an async handlebars helper for a given handlebars instance function asyncHelperWrapper(hbsInstance, name, fn) { - hbsInstance.registerAsyncHelper(name, function returnAsync(context, options, cb) { + hbsInstance.registerAsyncHelper(name, async function returnAsync(context, options, cb) { // Handle the case where we only get context and cb if (!cb) { cb = options; options = undefined; } - // 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) { - const wrappedErr = errors.utils.isGhostError(err) ? err : new errors.IncorrectUsageError({ - err: err, + try { + const response = await fn.call(this, context, options); + cb(response); + } catch (error) { + const wrappedErr = errors.utils.isGhostError(error) ? error : new errors.IncorrectUsageError({ + err: error, context: 'registerAsyncThemeHelper: ' + name, errorDetails: { - originalError: err + originalError: error } }); - const result = process.env.NODE_ENV === 'development' ? wrappedErr : ''; + const response = process.env.NODE_ENV === 'development' ? wrappedErr : ''; logging.error(wrappedErr); - cb(new hbsInstance.SafeString(result)); - }); + cb(new hbsInstance.SafeString(response)); + } }); }