From 8f44f69a805d8e5f608854405e450c2c3a870198 Mon Sep 17 00:00:00 2001 From: June Kelly Date: Thu, 22 Sep 2022 09:52:03 +0100 Subject: [PATCH] Merge pull request #9679 from overleaf/jk-fix-the-module-system [web] Fix how imports work in the Module system GitOrigin-RevId: 00cb3bfa19c6af979216b9d5e6104d489c18244b --- .../AuthenticationController.js | 3 +-- .../Features/Subscription/FeaturesUpdater.js | 2 +- .../app/src/Features/User/UserController.js | 3 +-- .../web/app/src/Features/User/UserUpdater.js | 3 +-- .../web/app/src/infrastructure/Modules.js | 23 ++++++++++++------- services/web/app/src/infrastructure/Server.js | 5 ++-- 6 files changed, 22 insertions(+), 17 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 5445e71101..1761620695 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -25,6 +25,7 @@ const { } = require('../../infrastructure/RequestContentTypeDetection') const { ParallelLoginError } = require('./AuthenticationErrors') const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper') +const Modules = require('../../infrastructure/Modules') function send401WithChallenge(res) { res.setHeader('WWW-Authenticate', 'OverleafLogin') @@ -119,7 +120,6 @@ const AuthenticationController = { const anonymousAnalyticsId = req.session.analyticsId const isNewUser = req.session.justRegistered || false - const Modules = require('../../infrastructure/Modules') Modules.hooks.fire( 'preFinishLogin', req, @@ -172,7 +172,6 @@ const AuthenticationController = { doPassportLogin(req, username, password, done) { const email = username.toLowerCase() - const Modules = require('../../infrastructure/Modules') Modules.hooks.fire( 'preDoPassportLogin', req, diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index 1eb948fc86..160fdf38db 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.js @@ -13,6 +13,7 @@ const InstitutionsFeatures = require('../Institutions/InstitutionsFeatures') const UserGetter = require('../User/UserGetter') const AnalyticsManager = require('../Analytics/AnalyticsManager') const Queues = require('../../infrastructure/Queues') +const Modules = require('../../infrastructure/Modules') /** * Enqueue a job for refreshing features for the given user @@ -52,7 +53,6 @@ async function refreshFeatures(userId, reason) { await UserFeaturesUpdater.promises.updateFeatures(userId, features) if (oldFeatures.dropbox === true && features.dropbox === false) { logger.debug({ userId }, '[FeaturesUpdater] must unlink dropbox') - const Modules = require('../../infrastructure/Modules') try { await Modules.promises.hooks.fire('removeDropbox', userId, reason) } catch (err) { diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index bd7e8d8e81..24453fdbde 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -21,6 +21,7 @@ const { expressify } = require('../../util/promises') const { acceptsJson, } = require('../../infrastructure/RequestContentTypeDetection') +const Modules = require('../../infrastructure/Modules') async function _sendSecurityAlertClearedSessions(user) { const emailOptions = { @@ -290,8 +291,6 @@ const UserController = { OError.tag(err, 'error unsubscribing to newsletter') return next(err) } - // TODO: figure out why things go wrong if we import at the top - const Modules = require('../../infrastructure/Modules') Modules.hooks.fire('newsletterUnsubscribed', user, err => { if (err) { OError.tag(err, 'error firing "newsletterUnsubscribed" hook') diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 03da274ff5..ab6ad1f7cc 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -17,6 +17,7 @@ const AnalyticsManager = require('../Analytics/AnalyticsManager') const SubscriptionLocator = require('../Subscription/SubscriptionLocator') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const _ = require('lodash') +const Modules = require('../../infrastructure/Modules') async function _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email) { // Send email to the following: @@ -218,8 +219,6 @@ async function setDefaultEmailAddress( ) } try { - // TODO: figure out why things go wrong if we import at the top - const Modules = require('../../infrastructure/Modules') await Modules.promises.hooks.fire('userEmailChanged', user, email) } catch (err) { logger.error( diff --git a/services/web/app/src/infrastructure/Modules.js b/services/web/app/src/infrastructure/Modules.js index cb2c8e99fa..73f65131af 100644 --- a/services/web/app/src/infrastructure/Modules.js +++ b/services/web/app/src/infrastructure/Modules.js @@ -8,9 +8,17 @@ const Settings = require('@overleaf/settings') const MODULE_BASE_PATH = Path.join(__dirname, '/../../../modules') const _modules = [] +let _modulesLoaded = false const _hooks = {} let _viewIncludes = {} +function modules() { + if (!_modulesLoaded) { + loadModules() + } + return _modules +} + function loadModules() { const settingsCheckModule = Path.join( MODULE_BASE_PATH, @@ -30,11 +38,12 @@ function loadModules() { loadedModule.name = moduleName _modules.push(loadedModule) } + _modulesLoaded = true attachHooks() } function applyRouter(webRouter, privateApiRouter, publicApiRouter) { - for (const module of _modules) { + for (const module of modules()) { if (module.router && module.router.apply) { module.router.apply(webRouter, privateApiRouter, publicApiRouter) } @@ -42,7 +51,7 @@ function applyRouter(webRouter, privateApiRouter, publicApiRouter) { } function applyNonCsrfRouter(webRouter, privateApiRouter, publicApiRouter) { - for (const module of _modules) { + for (const module of modules()) { if (module.nonCsrfRouter != null) { module.nonCsrfRouter.apply(webRouter, privateApiRouter, publicApiRouter) } @@ -58,7 +67,7 @@ function applyNonCsrfRouter(webRouter, privateApiRouter, publicApiRouter) { function loadViewIncludes(app) { _viewIncludes = {} - for (const module of _modules) { + for (const module of modules()) { const object = module.viewIncludes || {} for (const view in object) { const partial = object[view] @@ -82,7 +91,7 @@ function loadViewIncludes(app) { } function registerAppMiddleware(app) { - for (const module of _modules) { + for (const module of modules()) { if (module.appMiddleware) { module.appMiddleware(app) } @@ -104,7 +113,7 @@ function moduleIncludesAvailable(view) { function linkedFileAgentsIncludes() { const agents = {} - for (const module of _modules) { + for (const module of modules()) { for (const name in module.linkedFileAgents) { const agentFunction = module.linkedFileAgents[name] agents[name] = agentFunction() @@ -114,7 +123,7 @@ function linkedFileAgentsIncludes() { } function attachHooks() { - for (const module of _modules) { + for (const module of modules()) { if (module.hooks != null) { for (const hook in module.hooks) { const method = module.hooks[hook] @@ -163,5 +172,3 @@ module.exports = { }, }, } - -loadModules() diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 740e5122c4..63cee8a5dd 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -3,7 +3,6 @@ const express = require('express') const Settings = require('@overleaf/settings') const logger = require('@overleaf/logger') const metrics = require('@overleaf/metrics') -const expressLocals = require('./ExpressLocals') const Validation = require('./Validation') const csp = require('./CSP') const Router = require('../router') @@ -31,7 +30,6 @@ const ReferalConnect = require('../Features/Referal/ReferalConnect') const RedirectManager = require('./RedirectManager') const ProxyManager = require('./ProxyManager') const translations = require('./Translations') -const Modules = require('./Modules') const Views = require('./Views') const Features = require('./Features') @@ -44,6 +42,9 @@ const { hasAdminAccess, } = require('../Features/Helpers/AdminAuthorizationHelper') +const Modules = require('./Modules') +const expressLocals = require('./ExpressLocals') + const STATIC_CACHE_AGE = Settings.cacheStaticAssets ? oneDayInMilliseconds * 365 : 0