Merge pull request #9679 from overleaf/jk-fix-the-module-system

[web] Fix how imports work in the Module system

GitOrigin-RevId: 00cb3bfa19c6af979216b9d5e6104d489c18244b

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,

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) {

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')

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(

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()

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