Refactored theme service to use async/await

refs: https://github.com/TryGhost/Team/issues/831

- We prefer async/await over promise chains because it makes the code much more readable
- the Theme Service needs further work and this should make that work much easier
   - e.g. https://github.com/TryGhost/Team/issues/831
   - e.g. fixing up the index.js file

core/server/services/themes/loader.js

@@ -1,25 +1,19 @@
 const debug = require('@tryghost/debug')('themes');
-const config = require('../../../shared/config');
 const packageJSON = require('@tryghost/package-json');
+
+const config = require('../../../shared/config');
 const themeList = require('./list');
 
-const loadAllThemes = function loadAllThemes() {
+const loadAllThemes = async function loadAllThemes() {
-    return packageJSON
+    const themes = await packageJSON.readPackages(config.getContentPath('themes'));
-        .readPackages(config.getContentPath('themes'))
+    debug('loading themes', Object.keys(themes));
-        .then(function updateThemeList(themes) {
+    themeList.init(themes);
-            debug('loading themes', Object.keys(themes));
-
-            themeList.init(themes);
-        });
 };
 
-const loadOneTheme = function loadOneTheme(themeName) {
+const loadOneTheme = async function loadOneTheme(themeName) {
-    return packageJSON
+    const theme = await packageJSON.readPackage(config.getContentPath('themes'), themeName);
-        .readPackage(config.getContentPath('themes'), themeName)
+    debug('loaded one theme', themeName);
-        .then(function (readThemes) {
+    return themeList.set(themeName, theme[themeName]);
-            debug('loaded one theme', themeName);
-            return themeList.set(themeName, readThemes[themeName]);
-        });
 };
 
 module.exports = {

core/server/services/themes/storage.js

@@ -1,7 +1,11 @@
 const debug = require('@tryghost/debug')('themes');
 const fs = require('fs-extra');
+const ObjectID = require('bson-objectid');
+
+const tpl = require('@tryghost/tpl');
+const logging = require('@tryghost/logging');
+const errors = require('@tryghost/errors');
 
-const bridge = require('../../../bridge');
 const validate = require('./validate');
 const list = require('./list');
 const ThemeStorage = require('./ThemeStorage');
@@ -9,11 +13,7 @@ const themeLoader = require('./loader');
 const toJSON = require('./to-json');
 
 const settingsCache = require('../../../shared/settings-cache');
-const tpl = require('@tryghost/tpl');
+const bridge = require('../../../bridge');
-const logging = require('@tryghost/logging');
-const errors = require('@tryghost/errors');
-
-const ObjectID = require('bson-objectid');
 
 const messages = {
     themeDoesNotExist: 'Theme does not exist.',
@@ -32,20 +32,20 @@ const getStorage = () => {
 };
 
 module.exports = {
-    getZip: (themeName) => {
+    getZip: async (themeName) => {
         const theme = list.get(themeName);
 
         if (!theme) {
-            return Promise.reject(new errors.BadRequestError({
+            throw new errors.BadRequestError({
                 message: tpl(messages.invalidThemeName)
-            }));
+            });
         }
 
-        return getStorage().serve({
+        return await getStorage().serve({
             name: themeName
         });
     },
-    setFromZip: (zip) => {
+    setFromZip: async (zip) => {
         const shortName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
         const backupName = `${shortName}_${ObjectID()}`;
 
@@ -57,79 +57,69 @@ module.exports = {
         }
 
         let checkedTheme;
+        let overrideTheme;
         let renamedExisting = false;
 
-        return validate.checkSafe(zip, true)
+        try {
-            .then((_checkedTheme) => {
+            checkedTheme = await validate.checkSafe(zip, true);
-                checkedTheme = _checkedTheme;
+            const themeExists = await getStorage().exists(shortName);
+            // CASE: move the existing theme to a backup folder
+            if (themeExists) {
+                renamedExisting = true;
+                await getStorage().rename(shortName, backupName);
+            }
 
-                return getStorage().exists(shortName);
+            // CASE: store extracted theme
-            })
+            await getStorage().save({
-            .then((themeExists) => {
+                name: shortName,
-                // CASE: move the existing theme to a backup folder
+                path: checkedTheme.path
-                if (themeExists) {
+            });
-                    renamedExisting = true;
+            // CASE: loads the theme from the fs & sets the theme on the themeList
-                    return getStorage().rename(shortName, backupName);
+            const loadedTheme = await themeLoader.loadOneTheme(shortName);
-                }
+            overrideTheme = (shortName === settingsCache.get('active_theme'));
-            })
+            // CASE: if this is the active theme, we are overriding
-            .then(() => {
+            if (overrideTheme) {
-                // CASE: store extracted theme
+                debug('Activating theme (method C, on API "override")', shortName);
-                return getStorage().save({
+                bridge.activateTheme(loadedTheme, checkedTheme);
-                    name: shortName,
+            }
-                    path: checkedTheme.path
-                });
-            })
-            .then(() => {
-                // CASE: loads the theme from the fs & sets the theme on the themeList
-                return themeLoader.loadOneTheme(shortName);
-            })
-            .then((loadedTheme) => {
-                const overrideTheme = (shortName === settingsCache.get('active_theme'));
-                // CASE: if this is the active theme, we are overriding
-                if (overrideTheme) {
-                    debug('Activating theme (method C, on API "override")', shortName);
-                    bridge.activateTheme(loadedTheme, checkedTheme);
-                }
 
-                // @TODO: unify the name across gscan and Ghost!
+            // @TODO: unify the name across gscan and Ghost!
-                return {
+            return {
-                    themeOverridden: overrideTheme,
+                themeOverridden: overrideTheme,
-                    theme: toJSON(shortName, checkedTheme)
+                theme: toJSON(shortName, checkedTheme)
-                };
+            };
-            })
+        } catch (error) {
-            .catch((error) => {
+            // restore backup if we renamed an existing theme but saving failed
-                // restore backup if we renamed an existing theme but saving failed
+            if (renamedExisting) {
-                if (renamedExisting) {
+                return getStorage().exists(shortName).then((themeExists) => {
-                    return getStorage().exists(shortName).then((themeExists) => {
+                    if (!themeExists) {
-                        if (!themeExists) {
+                        return getStorage().rename(backupName, shortName).then(() => {
-                            return getStorage().rename(backupName, shortName).then(() => {
+                            throw error;
-                                throw error;
-                            });
-                        }
-                    });
-                }
-
-                throw error;
-            })
-            .finally(() => {
-                // @TODO: we should probably do this as part of saving the theme
-                // CASE: remove extracted dir from gscan happens in background
-                if (checkedTheme) {
-                    fs.remove(checkedTheme.path)
-                        .catch((err) => {
-                            logging.error(new errors.GhostError({err: err}));
                         });
-                }
+                    }
+                });
+            }
 
-                // CASE: remove the backup we created earlier
+            throw error;
-                getStorage()
+        } finally {
-                    .delete(backupName)
+            // @TODO: we should probably do this as part of saving the theme
+            // CASE: remove extracted dir from gscan happens in background
+            if (checkedTheme) {
+                fs.remove(checkedTheme.path)
                     .catch((err) => {
                         logging.error(new errors.GhostError({err: err}));
                     });
-            });
+            }
+
+            // CASE: remove the backup we created earlier
+            getStorage()
+                .delete(backupName)
+                .catch((err) => {
+                    logging.error(new errors.GhostError({err: err}));
+                });
+        }
     },
-    destroy: function (themeName) {
+    destroy: async function (themeName) {
         if (themeName === 'casper') {
             throw new errors.ValidationError({
                 message: tpl(messages.destroyCasper)
@@ -150,9 +140,8 @@ module.exports = {
             });
         }
 
-        return getStorage().delete(themeName)
+        let result = await getStorage().delete(themeName);
-            .then(() => {
+        list.del(themeName);
-                list.del(themeName);
+        return result;
-            });
     }
 };

core/server/services/themes/validate.js

@@ -16,63 +16,59 @@ const canActivate = function canActivate(checkedTheme) {
     return !checkedTheme.results.error.length || (config.get('env') === 'development') && !checkedTheme.results.hasFatalErrors;
 };
 
-const check = function check(theme, isZip) {
+const check = async function check(theme, isZip) {
     debug('Begin: Check');
     // gscan can slow down boot time if we require on boot, for now nest the require.
     const gscan = require('gscan');
-    let checkPromise;
+    let checkedTheme;
 
     if (isZip) {
         debug('zip mode');
-        checkPromise = gscan.checkZip(theme, {
+        checkedTheme = await gscan.checkZip(theme, {
             keepExtractedDir: true,
             checkVersion: 'canary'
         });
     } else {
         debug('non-zip mode');
-        checkPromise = gscan.check(theme.path, {
+        checkedTheme = await gscan.check(theme.path, {
             checkVersion: 'canary'
         });
     }
 
-    return checkPromise
+    checkedTheme = gscan.format(checkedTheme, {
-        .then(function resultHandler(checkedTheme) {
+        onlyFatalErrors: config.get('env') === 'production',
-            checkedTheme = gscan.format(checkedTheme, {
+        checkVersion: 'canary'
-                onlyFatalErrors: config.get('env') === 'production',
+    });
-                checkVersion: 'canary'
-            });
 
-            debug('End: Check');
+    debug('End: Check');
-            return checkedTheme;
+    return checkedTheme;
-        });
 };
 
-const checkSafe = function checkSafe(theme, isZip) {
+const checkSafe = async function checkSafe(theme, isZip) {
-    return check(theme, isZip)
+    const checkedTheme = await check(theme, isZip);
-        .then((checkedTheme) => {
-            if (canActivate(checkedTheme)) {
-                return checkedTheme;
-            }
 
-            // NOTE: When theme cannot be activated and gscan explicitly keeps extracted files (after
+    if (canActivate(checkedTheme)) {
-            //       being called with `keepExtractedDir: true`), this is the closes place for a cleanup.
+        return checkedTheme;
-            // TODO: The `keepExtractedDir` flag is the cause of confusion for when and where the cleanup
+    }
-            //       should be done. It's probably best if gscan is called directly with path to the extracted
-            //       directory, this would allow keeping gscan to do just one thing - validate the theme, and
-            //       file manipulations could be left to another module/library
-            if (isZip) {
-                fs.remove(checkedTheme.path);
-            }
 
-            return Promise.reject(new errors.ThemeValidationError({
+    // NOTE: When theme cannot be activated and gscan explicitly keeps extracted files (after
-                message: tpl(messages.invalidTheme),
+    //       being called with `keepExtractedDir: true`), this is the best place for a cleanup.
-                errorDetails: Object.assign(
+    // TODO: The `keepExtractedDir` flag is the cause of confusion for when and where the cleanup
-                    _.pick(checkedTheme, ['checkedVersion', 'name', 'path', 'version']), {
+    //       should be done. It's probably best if gscan is called directly with path to the extracted
-                        errors: checkedTheme.results.error
+    //       directory, this would allow keeping gscan to do just one thing - validate the theme, and
-                    }
+    //       file manipulations could be left to another module/library
-                )
+    if (isZip) {
-            }));
+        fs.remove(checkedTheme.path);
-        });
+    }
+
+    return Promise.reject(new errors.ThemeValidationError({
+        message: tpl(messages.invalidTheme),
+        errorDetails: Object.assign(
+            _.pick(checkedTheme, ['checkedVersion', 'name', 'path', 'version']), {
+                errors: checkedTheme.results.error
+            }
+        )
+    }));
 };
 
 module.exports.check = check;