Rework route service to prevent circular dependencies (#9229)

refs #9192, refs #9178  

After trying to progress with current implementation, it became clear that the route service can't control the boot sequence, because then we end up with circular dependencies between the route service and the channel service.

The route service now exposes:
-  a siteRouter 
- a way for apps to register routes.
- ParentRouter base class for other modules to use
- the registry

...

- moved the default route setup back to site/routes.js 🙈
- moved the parent channel router back to the channel service (this makes way more sense imo)
- this structure prevents circular dependencies
- split the registry out into it's own thing
- fixed-up various bits of tests and comments
- DEBUG will print a list of routes 🎉

core/server/services/channels/index.js

@@ -1,2 +1,13 @@
-module.exports.router = require('./router');
-module.exports.loader = require('./loader');
+/**
+ * # Channel Service
+ *
+ * The channel service is responsible for:
+ * - maintaining the config of available Channels
+ * - building out the logic of how an individual Channel works
+ * - providing a top level router as an entry point
+ *
+ * Everything else exposed via the Channel object
+ */
+
+// Exports the top-level router
+module.exports = require('./parent-router');

core/server/services/channels/parent-router.js

@@ -0,0 +1,19 @@
+var ParentRouter = require('../route').ParentRouter,
+    loader = require('./loader'),
+    channelRouter = require('./router');
+
+/**
+ * Channels Router
+ * Parent channels router will load & mount all routes when
+ * .router() is called. This allows for reloading.
+ */
+module.exports.router = function channelsRouter() {
+    var channelsRouter = new ParentRouter('channels');
+
+    loader.list().forEach(function (channel) {
+        // Create a new channelRouter, and mount it onto the parent at the correct route
+        channelsRouter.mountRouter(channel.route, channelRouter(channel));
+    });
+
+    return channelsRouter.router();
+};

core/server/services/route/ParentRouter.js

@@ -9,9 +9,8 @@
 
 var debug = require('ghost-ignition').debug('services:routes:ParentRouter'),
     express = require('express'),
-    // This is a shared global cache
-    // @TODO expand this as part of the route service
-    routes = [];
+    // This the route registry for the whole site
+    registry = require('./registry');
 /**
  * We expose a very limited amount of express.Router via specialist methods
  */
@@ -27,7 +26,7 @@ class ParentRouter {
             debug(this.name + ': mountRouter: ' + router.name);
             this._router.use(router);
         } else {
-            routes.push(path);
+            registry.set(this.name, path);
             debug(this.name + ': mountRouter: ' + router.name + ' at ' + path);
             this._router.use(path, router);
         }
@@ -35,7 +34,7 @@ class ParentRouter {
 
     mountRoute(path, controller) {
         debug(this.name + ': mountRoute for', path, controller.name);
-        routes.push(path);
+        registry.set(this.name, path);
         this._router.get(path, controller);
     }
 

core/server/services/route/app-router.js

@@ -1,4 +1,4 @@
-var Router = require('./base/Router'),
-    appRouter = new Router('apps');
+var ParentRouter = require('./ParentRouter'),
+    appRouter = new ParentRouter('apps');
 
 module.exports = appRouter;

core/server/services/route/channels-router.js

@@ -1,18 +0,0 @@
-var Router = require('./base/Router'),
-    channelService = require('../channels');
-
-/**
- * Channels Router
- * Parent channels router will load & mount all routes when
- * .router() is called. This allows for reloading.
- */
-module.exports.router = function channelsRouter() {
-    var channelsRouter = new Router('channels');
-
-    channelService.loader.list().forEach(function (channel) {
-        // Mount this channel router on the parent channels router
-        channelsRouter.mountRouter(channel.route, channelService.router(channel));
-    });
-
-    return channelsRouter.router();
-};

core/server/services/route/index.js

@@ -5,19 +5,18 @@
  * subrouters, or controllers mounted on them. There are not that many routes.
  *
  * The route service is intended to:
- * - handle the mounting of all the routes throughout the bootup sequence
  * - keep track of the registered routes, and what they have mounted on them
  * - provide a way for apps to register routes
- * - keep routes being served in a sane order
- *
- * The route service does not handle:
- * - redirects
- * - assets
- * These both happen prior to the routeService router being mounted
+ * - expose base classes & registry to the rest of Ghost
  */
 
+// This is the main router, that gets extended & mounted /site
+module.exports.siteRouter = require('./site-router');
+
 // We expose this via the App Proxy, so that Apps can register routes
 module.exports.appRouter = require('./app-router');
-// This is the main router, that gets mounted in the express app in /site
-module.exports.router = require('./site-router');
 
+// Classes for other parts of Ghost to extend
+module.exports.ParentRouter = require('./ParentRouter');
+
+module.exports.registry = require('./registry');

core/server/services/route/registry.js

@@ -0,0 +1,12 @@
+var _ = require('lodash'),
+    routes = [];
+
+module.exports = {
+    set(routerName, route) {
+        routes.push({route: route, from: routerName});
+    },
+
+    getAll() {
+        return _.cloneDeep(routes);
+    }
+};

core/server/services/route/site-router.js

@@ -1,48 +1,5 @@
-var Router = require('./base/Router'),
-    siteRouter = new Router('site'),
+// Site Router is the top level Router for the whole site
+var ParentRouter = require('./ParentRouter'),
+    siteRouter = new ParentRouter('site');
 
-    // Sub Routers
-    appRouter = require('./app-router'),
-    channelsRouter = require('./channels-router'),
-
-    // Controllers
-    controllers = require('../../controllers'),
-
-    // Utils for creating paths
-    // @TODO: refactor these away
-    config = require('../../config'),
-    utils = require('../../utils'),
-
-    _private = {};
-
-_private.mountDefaultRoutes = function mountDefaultRoutes() {
-    // @TODO move this path out of this file!
-    // Note this also exists in api/index.js
-    var previewRoute = utils.url.urlJoin('/', config.get('routeKeywords').preview, ':uuid', ':options?');
-
-    // Preview - register controller as route
-    // Ideal version, as we don't want these paths all over the place
-    // previewRoute = new Route('GET /:t_preview/:uuid/:options?', previewController);
-    // siteRouter.mountRoute(previewRoute);
-    // Orrrrr maybe preview should be an internal App??!
-    siteRouter.mountRoute(previewRoute, controllers.preview);
-
-    // Channels - register sub-router
-    // The purpose of having a parentRouter for channels, is so that we can load channels from wherever we want:
-    // config, settings, apps, etc, and that it will be possible for the router to be reloaded.
-    siteRouter.mountRouter(channelsRouter.router());
-
-    // Apps - register sub-router
-    // The purpose of having a parentRouter for apps, is that Apps can register a route whenever they want.
-    // Apps cannot yet deregister, it's complex to implement and I don't yet have a clear use-case for this.
-    siteRouter.mountRouter(appRouter.router());
-
-    // Default - register entry controller as route
-    siteRouter.mountRoute('*', controllers.entry);
-};
-
-module.exports = function router() {
-    _private.mountDefaultRoutes();
-
-    return siteRouter.router();
-};
+module.exports = siteRouter;

core/server/site/app.js

@@ -11,7 +11,7 @@ var debug = require('ghost-ignition').debug('blog'),
     sitemapHandler = require('../data/xml/sitemap/handler'),
 
     // Route Service
-    routeService = require('../services/route'),
+    siteRoutes = require('./routes'),
 
     // Global/shared middleware
     cacheControl = require('../middleware/cache-control'),
@@ -122,7 +122,7 @@ module.exports = function setupSiteApp() {
     debug('General middleware done');
 
     // Set up Frontend routes (including private blogging routes)
-    siteApp.use(routeService.router());
+    siteApp.use(siteRoutes());
 
     // ### Error handlers
     siteApp.use(errorHandler.pageNotFound);

core/server/site/routes.js

@@ -0,0 +1,46 @@
+var debug = require('ghost-ignition').debug('site:routes'),
+
+    routeService = require('../services/route'),
+    siteRouter = routeService.siteRouter,
+
+    // Sub Routers
+    appRouter = routeService.appRouter,
+    channelsService = require('../services/channels'),
+
+    // Controllers
+    controllers = require('../controllers'),
+
+    // Utils for creating paths
+    // @TODO: refactor these away
+    config = require('../config'),
+    utils = require('../utils');
+
+module.exports = function siteRoutes() {
+    // @TODO move this path out of this file!
+    // Note this also exists in api/index.js
+    var previewRoute = utils.url.urlJoin('/', config.get('routeKeywords').preview, ':uuid', ':options?');
+
+    // Preview - register controller as route
+    // Ideal version, as we don't want these paths all over the place
+    // previewRoute = new Route('GET /:t_preview/:uuid/:options?', previewController);
+    // siteRouter.mountRoute(previewRoute);
+    // Orrrrr maybe preview should be an internal App??!
+    siteRouter.mountRoute(previewRoute, controllers.preview);
+
+    // Channels - register sub-router
+    // The purpose of having a parentRouter for channels, is so that we can load channels from wherever we want:
+    // config, settings, apps, etc, and that it will be possible for the router to be reloaded.
+    siteRouter.mountRouter(channelsService.router());
+
+    // Apps - register sub-router
+    // The purpose of having a parentRouter for apps, is that Apps can register a route whenever they want.
+    // Apps cannot yet deregister, it's complex to implement and I don't yet have a clear use-case for this.
+    siteRouter.mountRouter(appRouter.router());
+
+    // Default - register entry controller as route
+    siteRouter.mountRoute('*', controllers.entry);
+
+    debug('Routes:', routeService.registry.getAll());
+
+    return siteRouter.router();
+};

core/test/unit/services/channels/custom_channels_spec.js

@@ -4,7 +4,7 @@ var should = require('should'),  // jshint ignore:line
 
     // Stuff we are testing
     channelLoader = require('../../../../server/services/channels/loader'),
-    channelsParentRouter = require('../../../../server/services/route/channels-router'),
+    channelsParentRouter = require('../../../../server/services/channels'),
     channelUtils = require('../../../utils/channelUtils'),
     Channel = channelUtils.Channel,
 

core/test/unit/services/channels/parent-router_spec.js

@@ -4,7 +4,7 @@ var should = require('should'),
     _ = require('lodash'),
 
     // Stuff we are testing
-    channelsParentRouter = require('../../../../server/services/route/channels-router'),
+    channelsParentRouter = require('../../../../server/services/channels'),
     api = require('../../../../server/api'),
     themes = require('../../../../server/themes'),
     sandbox = sinon.sandbox.create();