From 18344a16e28cf8bb8b61c693bcb12f80147d3e8b Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 14 Oct 2021 19:10:36 +0200 Subject: [PATCH] Removed event chain caused by settings date update refs https://linear.app/tryghost/issue/CORE-104/decouple-frontend-routing-events-from-urlserver-events - The 'settings.timezone.edited' event triggers a roundtrip chain of calls in the frontend routing to the url services. It was all handled by event listeners and handler that clearly don't belong there. - Extracted event realted listeners/handlers into methods and moved most of the logic to the "bootstrap" module, which soon is going to become a "RoutesManger" - The result of this refactor - no more events going back and forth between frontend routing and the backend! --- core/bridge.js | 8 ++ .../services/routing/CollectionRouter.js | 47 ------------ core/frontend/services/routing/bootstrap.js | 40 ++++++++++ core/frontend/services/routing/registry.js | 13 ++++ core/frontend/services/url/UrlGenerator.js | 28 +++---- core/frontend/services/url/UrlService.js | 9 +++ .../services/routing/CollectionRouter.test.js | 58 --------------- .../services/routing/bootstrap.test.js | 74 +++++++++++++++++++ .../services/url/UrlGenerator.test.js | 4 +- 9 files changed, 160 insertions(+), 121 deletions(-) create mode 100644 test/unit/frontend/services/routing/bootstrap.test.js diff --git a/core/bridge.js b/core/bridge.js index a08ab60b70..d18cf99ef8 100644 --- a/core/bridge.js +++ b/core/bridge.js @@ -16,6 +16,7 @@ const config = require('./shared/config'); const logging = require('@tryghost/logging'); const tpl = require('@tryghost/tpl'); const themeEngine = require('./frontend/services/theme-engine'); +const frontendRouting = require('./frontend/services/routing/bootstrap'); const settingsCache = require('./shared/settings-cache'); // Listen to settings.lang.edited, similar to the member service and models/base/listeners @@ -35,6 +36,13 @@ class Bridge { debug('Active theme init18n'); this.getActiveTheme().initI18n({locale: model.get('value')}); }); + + // NOTE: eventually this event should somehow be listened on and handled by the URL Service + // for now this eliminates the need for the frontend routing to listen to + // server events + events.on('settings.timezone.edited', (model) => { + frontendRouting.handleTimezoneEdit(model); + }); } getActiveTheme() { diff --git a/core/frontend/services/routing/CollectionRouter.js b/core/frontend/services/routing/CollectionRouter.js index 4d496e7fde..8e494240e9 100644 --- a/core/frontend/services/routing/CollectionRouter.js +++ b/core/frontend/services/routing/CollectionRouter.js @@ -7,9 +7,6 @@ const middlewares = require('./middlewares'); const RSSRouter = require('./RSSRouter'); const bootstrap = require('./bootstrap'); -// This emits its own routing events AND listens to settings.timezone.edited :( -const events = require('../../../server/lib/common/events'); - /** * @description Collection Router for post resource. * @@ -60,7 +57,6 @@ class CollectionRouter extends ParentRouter { debug(this.name, this.route, this.permalinks); this._registerRoutes(); - this._listeners(); } /** @@ -128,43 +124,6 @@ class CollectionRouter extends ParentRouter { next(); } - /** - * @description This router has listeners to react on changes which happen in Ghost. - * @private - */ - _listeners() { - /** - * CASE: timezone changes - * - * If your permalink contains a date reference, we have to regenerate the urls. - * - * e.g. /:year/:month/:day/:slug/ or /:day/:slug/ - */ - this._onTimezoneEditedListener = this._onTimezoneEdited.bind(this); - events.on('settings.timezone.edited', this._onTimezoneEditedListener); - } - - /** - * @description Helper function to handle a timezone change. - * @param settingModel - * @private - */ - _onTimezoneEdited(settingModel) { - const newTimezone = settingModel.attributes.value; - const previousTimezone = settingModel._previousAttributes.value; - - if (newTimezone === previousTimezone) { - return; - } - - if (this.getPermalinks().getValue().match(/:year|:month|:day/)) { - debug('_onTimezoneEdited: trigger regeneration'); - - // @NOTE: The connected url generator will listen on this event and regenerate urls. - this.emit('updated'); - } - } - /** * @description Get resource type of this router (always "posts") * @returns {string} @@ -197,12 +156,6 @@ class CollectionRouter extends ParentRouter { return urlUtils.createUrl(urlUtils.urlJoin(this.route.value, this.rssRouter.route.value), options.absolute, options.secure); } - - reset() { - if (this._onTimezoneEditedListener) { - events.removeListener('settings.timezone.edited', this._onTimezoneEditedListener); - } - } } module.exports = CollectionRouter; diff --git a/core/frontend/services/routing/bootstrap.js b/core/frontend/services/routing/bootstrap.js index 0855a4ac07..764aa8216b 100644 --- a/core/frontend/services/routing/bootstrap.js +++ b/core/frontend/services/routing/bootstrap.js @@ -14,6 +14,8 @@ const events = require('../../../server/lib/common/events'); const defaultApiVersion = 'v4'; +// This registry thing should be passed into here as a dependency once the module +// is rewritten into the class + DI pattern const registry = require('./registry'); let siteRouter; let _urlService; @@ -119,6 +121,38 @@ const start = (apiVersion, routerSettings) => { debug('Routes:', registry.getAllRoutes()); }; +/** + * This is a glue code to keep the implementation of routers away from + * this sort of logic. Ideally this method should not be ever called + * and handled completely on the URL Service layer without touching the frontend + * @param {Object} settingModel instance of the settings model + * @returns {void} + */ +const handleTimezoneEdit = (settingModel) => { + const newTimezone = settingModel.attributes.value; + const previousTimezone = settingModel._previousAttributes.value; + + if (newTimezone === previousTimezone) { + return; + } + + /** + * CASE: timezone changes + * + * If your permalink contains a date reference, we have to regenerate the urls. + * + * e.g. /:year/:month/:day/:slug/ or /:day/:slug/ + */ + + // NOTE: timezone change only affects the collection router with dated permalinks + const collectionRouter = registry.getRouterByName('CollectionRouter'); + if (collectionRouter.getPermalinks().getValue().match(/:year|:month|:day/)) { + debug('handleTimezoneEdit: trigger regeneration'); + + this.internal.routerUpdated(collectionRouter); + } +}; + module.exports.internal = { owns: (routerId, id) => { return _urlService.owns(routerId, id); @@ -135,7 +169,13 @@ module.exports.internal = { events.emit('router.created', this); _urlService.onRouterAddedType(router); + }, + routerUpdated: (router) => { + _urlService.onRouterUpdated(router); } }; + +// Public API methods called out by the backend module.exports.init = init; module.exports.start = start; +module.exports.handleTimezoneEdit = handleTimezoneEdit; diff --git a/core/frontend/services/routing/registry.js b/core/frontend/services/routing/registry.js index abda8ee16f..c71c6840b6 100644 --- a/core/frontend/services/routing/registry.js +++ b/core/frontend/services/routing/registry.js @@ -41,6 +41,19 @@ module.exports = { return routers[name]; }, + /** + * Gets a router by it's internal router name + * @param {String} name internal router name + * @returns {Express-Router} + */ + getRouterByName(name) { + for (let routerKey in routers) { + if (routers[routerKey].name === name) { + return routers[routerKey]; + } + } + }, + /** * * diff --git a/core/frontend/services/url/UrlGenerator.js b/core/frontend/services/url/UrlGenerator.js index bb9ea39b6b..0897fa871f 100644 --- a/core/frontend/services/url/UrlGenerator.js +++ b/core/frontend/services/url/UrlGenerator.js @@ -67,25 +67,25 @@ class UrlGenerator { this._listeners(); } + /** + * @NOTE: currently only used if the permalink setting changes and it's used for this url generator. + * @TODO: https://github.com/TryGhost/Ghost/issues/10699 + */ + regenerateResources() { + const myResources = this.urls.getByGeneratorId(this.uid); + + myResources.forEach((object) => { + this.urls.removeResourceId(object.resource.data.id); + object.resource.release(); + this._try(object.resource); + }); + } + /** * @description Helper function to register listeners for each url generator instance. * @private */ _listeners() { - /** - * @NOTE: currently only used if the permalink setting changes and it's used for this url generator. - * @TODO: https://github.com/TryGhost/Ghost/issues/10699 - */ - this.router.addListener('updated', () => { - const myResources = this.urls.getByGeneratorId(this.uid); - - myResources.forEach((object) => { - this.urls.removeResourceId(object.resource.data.id); - object.resource.release(); - this._try(object.resource); - }); - }); - /** * Listen on two events: * diff --git a/core/frontend/services/url/UrlService.js b/core/frontend/services/url/UrlService.js index b1c56e8f46..54fa4d832a 100644 --- a/core/frontend/services/url/UrlService.js +++ b/core/frontend/services/url/UrlService.js @@ -89,6 +89,15 @@ class UrlService { this.urlGenerators.push(urlGenerator); } + /** + * @description Router update handler - regenerates it's resources + * @param {ExpressRouter} router + */ + onRouterUpdated(router) { + const generator = this.urlGenerators.find(g => g.router.id === router.id); + generator.regenerateResources(); + } + /** * @description If the API version in the theme config changes, we have to reset urls and resources. * @private diff --git a/test/unit/frontend/services/routing/CollectionRouter.test.js b/test/unit/frontend/services/routing/CollectionRouter.test.js index df6c0cdd3e..ccea2458ca 100644 --- a/test/unit/frontend/services/routing/CollectionRouter.test.js +++ b/test/unit/frontend/services/routing/CollectionRouter.test.js @@ -179,62 +179,4 @@ describe('UNIT - services/routing/CollectionRouter', function () { }); }); }); - - describe('timezone changes', function () { - describe('no dated permalink', function () { - it('default', function () { - const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:slug/'}, RESOURCE_CONFIG); - - sinon.stub(collectionRouter, 'emit'); - - events.on.args[0][1]({ - attributes: {value: 'America/Los_Angeles'}, - _previousAttributes: {value: 'Europe/London'} - }); - - collectionRouter.emit.called.should.be.false(); - }); - - it('tz has not changed', function () { - const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:slug/'}, RESOURCE_CONFIG); - - sinon.stub(collectionRouter, 'emit'); - - events.on.args[0][1]({ - attributes: {value: 'America/Los_Angeles'}, - _previousAttributes: {value: 'America/Los_Angeles'} - }); - - collectionRouter.emit.called.should.be.false(); - }); - }); - - describe('with dated permalink', function () { - it('default', function () { - const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:year/:slug/'}, RESOURCE_CONFIG); - - sinon.stub(collectionRouter, 'emit'); - - events.on.args[0][1]({ - attributes: {value: 'America/Los_Angeles'}, - _previousAttributes: {value: 'Europe/London'} - }); - - collectionRouter.emit.calledOnce.should.be.true(); - }); - - it('tz has not changed', function () { - const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:year/:slug/'}, RESOURCE_CONFIG); - - sinon.stub(collectionRouter, 'emit'); - - events.on.args[0][1]({ - attributes: {value: 'America/Los_Angeles'}, - _previousAttributes: {value: 'America/Los_Angeles'} - }); - - collectionRouter.emit.called.should.be.false(); - }); - }); - }); }); diff --git a/test/unit/frontend/services/routing/bootstrap.test.js b/test/unit/frontend/services/routing/bootstrap.test.js new file mode 100644 index 0000000000..2bf75c39b9 --- /dev/null +++ b/test/unit/frontend/services/routing/bootstrap.test.js @@ -0,0 +1,74 @@ +const should = require('should'); +const sinon = require('sinon'); +const CollectionRouter = require('../../../../../core/frontend/services/routing/CollectionRouter'); +const bootstrap = require('../../../../../core/frontend/services/routing/bootstrap'); +const registry = require('../../../../../core/frontend/services/routing/registry'); + +const RESOURCE_CONFIG = {QUERY: {post: {controller: 'posts', resource: 'posts'}}}; + +describe('UNIT: services/routing/bootstrap', function () { + let routesUpdatedStub; + + beforeEach(function () { + sinon.stub(bootstrap.internal, 'routerCreated').returns(); + routesUpdatedStub = sinon.stub(bootstrap.internal, 'routerUpdated').returns(); + }); + + afterEach(function () { + sinon.restore(); + }); + + describe('timezone changes', function () { + describe('no dated permalink', function () { + it('default', function () { + const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:slug/'}, RESOURCE_CONFIG); + sinon.stub(registry, 'getRouterByName').withArgs('CollectionRouter').returns(collectionRouter); + + bootstrap.handleTimezoneEdit({ + attributes: {value: 'America/Los_Angeles'}, + _previousAttributes: {value: 'Europe/London'} + }); + + routesUpdatedStub.called.should.be.false(); + }); + + it('tz has not changed', function () { + const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:slug/'}, RESOURCE_CONFIG); + sinon.stub(registry, 'getRouterByName').withArgs('CollectionRouter').returns(collectionRouter); + + bootstrap.handleTimezoneEdit({ + attributes: {value: 'America/Los_Angeles'}, + _previousAttributes: {value: 'America/Los_Angeles'} + }); + + routesUpdatedStub.called.should.be.false(); + }); + }); + + describe('with dated permalink', function () { + it('default', function () { + const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:year/:slug/'}, RESOURCE_CONFIG); + sinon.stub(registry, 'getRouterByName').withArgs('CollectionRouter').returns(collectionRouter); + + bootstrap.handleTimezoneEdit({ + attributes: {value: 'America/Los_Angeles'}, + _previousAttributes: {value: 'Europe/London'} + }); + + routesUpdatedStub.called.should.be.true(); + }); + + it('tz has not changed', function () { + const collectionRouter = new CollectionRouter('/magic/', {permalink: '/:year/:slug/'}, RESOURCE_CONFIG); + sinon.stub(registry, 'getRouterByName').withArgs('CollectionRouter').returns(collectionRouter); + + bootstrap.handleTimezoneEdit({ + attributes: {value: 'America/Los_Angeles'}, + _previousAttributes: {value: 'America/Los_Angeles'} + }); + + routesUpdatedStub.called.should.be.false(); + }); + }); + }); +}); diff --git a/test/unit/frontend/services/url/UrlGenerator.test.js b/test/unit/frontend/services/url/UrlGenerator.test.js index 662aa88be7..daf10be712 100644 --- a/test/unit/frontend/services/url/UrlGenerator.test.js +++ b/test/unit/frontend/services/url/UrlGenerator.test.js @@ -61,7 +61,6 @@ describe('Unit: services/url/UrlGenerator', function () { const urlGenerator = new UrlGenerator(router, queue); queue.register.calledTwice.should.be.true(); - router.addListener.calledOnce.should.be.true(); should.not.exist(urlGenerator.filter); }); @@ -94,7 +93,8 @@ describe('Unit: services/url/UrlGenerator', function () { id: 'object-id-1' }; - router.addListener.args[0][1](); + urlGenerator.regenerateResources(); + urls.removeResourceId.calledTwice.should.be.true(); resource.release.calledOnce.should.be.true(); resource2.release.calledOnce.should.be.true();