From add30f3d5be6f71bfad11dff04afbf330fbfc927 Mon Sep 17 00:00:00 2001 From: Naz Date: Wed, 13 Oct 2021 16:14:33 +0200 Subject: [PATCH] Decoupled frontend routing from url service refs https://linear.app/tryghost/issue/CORE-103/decouple-internal-frontend-code-from-url-module - By becoming a parameter in the routing bootstrap process URL is Service no longer a "require" inside the frontend controllers but rather becomes a part of the "internal API" of the bootstrapper. This is not the end form of it, rather a step closer to decouplint routing from the URL serivce. - The bootstrap module needs a facelift to have cleaner distinction between init/start methods. This is left for another time --- core/frontend/services/routing/bootstrap.js | 15 ++++++++- .../routing/controllers/collection.js | 4 +-- .../routing/controllers/email-post.js | 4 +-- .../services/routing/controllers/entry.js | 4 +-- .../services/routing/controllers/preview.js | 4 +-- core/server/web/site/routes.js | 3 ++ .../routing/controllers/collection.test.js | 33 ++++++++++--------- .../routing/controllers/entry.test.js | 12 +++---- 8 files changed, 48 insertions(+), 31 deletions(-) diff --git a/core/frontend/services/routing/bootstrap.js b/core/frontend/services/routing/bootstrap.js index d78166824b..79f482ac1f 100644 --- a/core/frontend/services/routing/bootstrap.js +++ b/core/frontend/services/routing/bootstrap.js @@ -16,6 +16,7 @@ const defaultApiVersion = 'v4'; const registry = require('./registry'); let siteRouter; +let _urlService; /** * @description The `init` function will return the wrapped parent express router and will start creating all @@ -29,7 +30,8 @@ let siteRouter; * @param {Object} options * @returns {ExpressRouter} */ -const init = ({start = false, routerSettings, apiVersion}) => { +const init = ({start = false, routerSettings, apiVersion, urlService}) => { + _urlService = urlService; debug('bootstrap init', start, apiVersion, routerSettings); registry.resetAllRouters(); @@ -113,5 +115,16 @@ const start = (apiVersion, routerSettings) => { debug('Routes:', registry.getAllRoutes()); }; +module.exports.internal = { + owns: (routerId, id) => { + return _urlService.owns(routerId, id); + }, + getUrlByResourceId: (id, options) => { + return _urlService.getUrlByResourceId(id, options); + }, + getResourceById: (resourceId) => { + return _urlService.getResourceById(resourceId); + } +}; module.exports.init = init; module.exports.start = start; diff --git a/core/frontend/services/routing/controllers/collection.js b/core/frontend/services/routing/controllers/collection.js index f8b39b90d3..c745e52c66 100644 --- a/core/frontend/services/routing/controllers/collection.js +++ b/core/frontend/services/routing/controllers/collection.js @@ -3,7 +3,7 @@ const debug = require('@tryghost/debug')('services:routing:controllers:collectio const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const security = require('@tryghost/security'); -const urlService = require('../../url'); +const bootstrap = require('../bootstrap'); const themeEngine = require('../../theme-engine'); const helpers = require('../helpers'); @@ -72,7 +72,7 @@ module.exports = function collectionController(req, res, next) { * People should always invert their filters to ensure that the database query loads unique posts per collection. */ result.posts = _.filter(result.posts, (post) => { - if (urlService.owns(res.routerOptions.identifier, post.id)) { + if (bootstrap.internal.owns(res.routerOptions.identifier, post.id)) { return post; } diff --git a/core/frontend/services/routing/controllers/email-post.js b/core/frontend/services/routing/controllers/email-post.js index 804aeecb33..5d71514709 100644 --- a/core/frontend/services/routing/controllers/email-post.js +++ b/core/frontend/services/routing/controllers/email-post.js @@ -1,6 +1,6 @@ const debug = require('@tryghost/debug')('services:routing:controllers:emailpost'); const config = require('../../../../shared/config'); -const urlService = require('../../url'); +const bootstrap = require('../bootstrap'); const urlUtils = require('../../../../shared/url-utils'); const helpers = require('../helpers'); @@ -48,7 +48,7 @@ module.exports = function emailPostController(req, res, next) { } if (post.status === 'published') { - return urlUtils.redirect301(res, urlService.getUrlByResourceId(post.id, {withSubdirectory: true})); + return urlUtils.redirect301(res, bootstrap.internal.getUrlByResourceId(post.id, {withSubdirectory: true})); } if (res.locals.apiVersion !== 'v0.1' && res.locals.apiVersion !== 'v2') { diff --git a/core/frontend/services/routing/controllers/entry.js b/core/frontend/services/routing/controllers/entry.js index 28f6157804..df888ea03a 100644 --- a/core/frontend/services/routing/controllers/entry.js +++ b/core/frontend/services/routing/controllers/entry.js @@ -1,7 +1,7 @@ const debug = require('@tryghost/debug')('services:routing:controllers:entry'); const url = require('url'); const config = require('../../../../shared/config'); -const urlService = require('../../../services/url'); +const bootstrap = require('../bootstrap'); const urlUtils = require('../../../../shared/url-utils'); const helpers = require('../helpers'); @@ -60,7 +60,7 @@ module.exports = function entryController(req, res, next) { * * That's why we have to check against the router type. */ - if (urlService.getResourceById(entry.id).config.type !== res.routerOptions.resourceType) { + if (bootstrap.internal.getResourceById(entry.id).config.type !== res.routerOptions.resourceType) { debug('not my resource type'); return next(); } diff --git a/core/frontend/services/routing/controllers/preview.js b/core/frontend/services/routing/controllers/preview.js index decd5e1a64..5384b4e2cb 100644 --- a/core/frontend/services/routing/controllers/preview.js +++ b/core/frontend/services/routing/controllers/preview.js @@ -1,6 +1,6 @@ const debug = require('@tryghost/debug')('services:routing:controllers:preview'); const config = require('../../../../shared/config'); -const urlService = require('../../url'); +const bootstrap = require('../bootstrap'); const urlUtils = require('../../../../shared/url-utils'); const helpers = require('../helpers'); @@ -50,7 +50,7 @@ module.exports = function previewController(req, res, next) { } if (post.status === 'published') { - return urlUtils.redirect301(res, urlService.getUrlByResourceId(post.id, {withSubdirectory: true})); + return urlUtils.redirect301(res, bootstrap.internal.getUrlByResourceId(post.id, {withSubdirectory: true})); } if (res.locals.apiVersion !== 'v0.1' && res.locals.apiVersion !== 'v2') { diff --git a/core/server/web/site/routes.js b/core/server/web/site/routes.js index 3fb0bc9c2d..5c343704ad 100644 --- a/core/server/web/site/routes.js +++ b/core/server/web/site/routes.js @@ -1,9 +1,12 @@ const debug = require('@tryghost/debug')('routing'); const routing = require('../../../frontend/services/routing'); +// NOTE: temporary import from the frontend, will become a backend service soon +const urlService = require('../../../frontend/services/url'); const routeSettings = require('../../services/route-settings'); module.exports = function siteRoutes(options = {}) { debug('site Routes', options); options.routerSettings = routeSettings.loadRouteSettingsSync(); + options.urlService = urlService; return routing.bootstrap.init(options); }; diff --git a/test/unit/frontend/services/routing/controllers/collection.test.js b/test/unit/frontend/services/routing/controllers/collection.test.js index 54b42ddddc..a21d1173b8 100644 --- a/test/unit/frontend/services/routing/controllers/collection.test.js +++ b/test/unit/frontend/services/routing/controllers/collection.test.js @@ -4,7 +4,7 @@ const sinon = require('sinon'); const testUtils = require('../../../../../utils'); const security = require('@tryghost/security'); const themeEngine = require('../../../../../../core/frontend/services/theme-engine'); -const urlService = require('../../../../../../core/frontend/services/url'); +const bootstrap = require('../../../../../../core/frontend/services/routing/bootstrap'); const controllers = require('../../../../../../core/frontend/services/routing/controllers'); const helpers = require('../../../../../../core/frontend/services/routing/helpers'); @@ -23,6 +23,7 @@ describe('Unit - services/routing/controllers/collection', function () { let renderStub; let posts; let postsPerPage; + let ownsStub; beforeEach(function () { postsPerPage = 5; @@ -55,8 +56,8 @@ describe('Unit - services/routing/controllers/collection', function () { sinon.stub(helpers, 'renderEntries').returns(renderStub); - sinon.stub(urlService, 'owns'); - urlService.owns.withArgs('identifier', posts[0].id).returns(true); + ownsStub = sinon.stub(bootstrap.internal, 'owns'); + ownsStub.withArgs('identifier', posts[0].id).returns(true); req = { path: '/', @@ -93,7 +94,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledOnce.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -116,7 +117,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledOnce.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -141,7 +142,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledOnce.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -167,7 +168,7 @@ describe('Unit - services/routing/controllers/collection', function () { fetchDataStub.calledOnce.should.be.true(); renderStub.calledOnce.should.be.false(); secureStub.calledOnce.should.be.false(); - urlService.owns.calledOnce.should.be.false(); + ownsStub.calledOnce.should.be.false(); done(); }); }); @@ -190,7 +191,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.true(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledOnce.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -213,7 +214,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledOnce.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -237,7 +238,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledTwice.should.be.true(); - urlService.owns.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); done(); }).catch(done); }); @@ -252,11 +253,11 @@ describe('Unit - services/routing/controllers/collection', function () { res.routerOptions.filter = 'featured:true'; - urlService.owns.reset(); - urlService.owns.withArgs('identifier', posts[0].id).returns(false); - urlService.owns.withArgs('identifier', posts[1].id).returns(true); - urlService.owns.withArgs('identifier', posts[2].id).returns(false); - urlService.owns.withArgs('identifier', posts[3].id).returns(false); + ownsStub.reset(); + ownsStub.withArgs('identifier', posts[0].id).returns(false); + ownsStub.withArgs('identifier', posts[1].id).returns(true); + ownsStub.withArgs('identifier', posts[2].id).returns(false); + ownsStub.withArgs('identifier', posts[3].id).returns(false); fetchDataStub.withArgs({page: 1, slug: undefined, limit: postsPerPage}, res.routerOptions) .resolves({ @@ -276,7 +277,7 @@ describe('Unit - services/routing/controllers/collection', function () { security.string.safe.calledOnce.should.be.false(); fetchDataStub.calledOnce.should.be.true(); secureStub.calledTwice.should.be.true(); - urlService.owns.callCount.should.eql(4); + ownsStub.callCount.should.eql(4); done(); }).catch(done); }); diff --git a/test/unit/frontend/services/routing/controllers/entry.test.js b/test/unit/frontend/services/routing/controllers/entry.test.js index 1de78fd3fb..9ab1b4b730 100644 --- a/test/unit/frontend/services/routing/controllers/entry.test.js +++ b/test/unit/frontend/services/routing/controllers/entry.test.js @@ -2,8 +2,8 @@ const should = require('should'); const sinon = require('sinon'); const testUtils = require('../../../../../utils'); const configUtils = require('../../../../../utils/configUtils'); -const urlService = require('../../../../../../core/frontend/services/url'); const urlUtils = require('../../../../../../core/shared/url-utils'); +const bootstrap = require('../../../../../../core/frontend/services/routing/bootstrap'); const controllers = require('../../../../../../core/frontend/services/routing/controllers'); const helpers = require('../../../../../../core/frontend/services/routing/helpers'); const EDITOR_URL = `/#/editor/post/`; @@ -41,7 +41,7 @@ describe('Unit - services/routing/controllers/entry', function () { sinon.stub(urlUtils, 'redirectToAdmin'); sinon.stub(urlUtils, 'redirect301'); - sinon.stub(urlService, 'getResourceById'); + sinon.stub(bootstrap.internal, 'getResourceById'); req = { path: '/', @@ -81,7 +81,7 @@ describe('Unit - services/routing/controllers/entry', function () { res.routerOptions.resourceType = 'posts'; - urlService.getResourceById.withArgs(post.id).returns({ + bootstrap.internal.getResourceById.withArgs(post.id).returns({ config: { type: 'posts' } @@ -162,7 +162,7 @@ describe('Unit - services/routing/controllers/entry', function () { req.path = post.url; res.routerOptions.resourceType = 'posts'; - urlService.getResourceById.withArgs(post.id).returns({ + bootstrap.internal.getResourceById.withArgs(post.id).returns({ config: { type: 'pages' } @@ -186,7 +186,7 @@ describe('Unit - services/routing/controllers/entry', function () { res.routerOptions.resourceType = 'posts'; - urlService.getResourceById.withArgs(post.id).returns({ + bootstrap.internal.getResourceById.withArgs(post.id).returns({ config: { type: 'posts' } @@ -215,7 +215,7 @@ describe('Unit - services/routing/controllers/entry', function () { res.routerOptions.resourceType = 'posts'; - urlService.getResourceById.withArgs(post.id).returns({ + bootstrap.internal.getResourceById.withArgs(post.id).returns({ config: { type: 'posts' }