Merge pull request #17430 from overleaf/dp-callbackify-class

Add callbackifyClass utility

GitOrigin-RevId: 762b800ce0eff2f146147908838162f7d32bd855

libraries/promise-utils/index.js

@@ -8,6 +8,7 @@ module.exports = {
   promisifyMultiResult,
   callbackify,
   callbackifyAll,
+  callbackifyClass,
   callbackifyMultiResult,
   expressify,
   expressifyErrorHandler,
@@ -177,6 +178,34 @@ function callbackifyAll(module, opts = {}) {
   return callbacks
 }
 
+/**
+ * Callbackify all methods in a class.
+ *
+ * Options are the same as for callbackifyAll
+ */
+function callbackifyClass(cls, opts = {}) {
+  const callbackified = class extends cls {}
+  const { without = [], multiResult = {} } = opts
+  for (const propName of Object.getOwnPropertyNames(cls.prototype)) {
+    if (propName === 'constructor' || without.includes(propName)) {
+      continue
+    }
+    const propValue = cls.prototype[propName]
+    if (typeof propValue !== 'function') {
+      continue
+    }
+    if (multiResult[propName] != null) {
+      callbackified.prototype[propName] = callbackifyMultiResult(
+        propValue,
+        multiResult[propName]
+      )
+    } else {
+      callbackified.prototype[propName] = callbackify(propValue)
+    }
+  }
+  return callbackified
+}
+
 /**
  * Reverse the effect of `promisifyMultiResult`.
  *
@@ -186,7 +215,7 @@ function callbackifyAll(module, opts = {}) {
 function callbackifyMultiResult(fn, resultNames) {
   function callbackified(...args) {
     const [callback] = args.splice(-1)
-    fn(...args)
+    fn.apply(this, args)
       .then(result => {
         const cbResults = resultNames.map(resultName => result[resultName])
         callback(null, ...cbResults)

libraries/promise-utils/test/unit/PromiseUtilsTests.js

@@ -3,6 +3,7 @@ const {
   promisifyAll,
   promisifyClass,
   callbackifyMultiResult,
+  callbackifyClass,
   callbackifyAll,
   expressify,
   expressifyErrorHandler,
@@ -327,6 +328,108 @@ describe('callbackifyAll', function () {
   })
 })
 
+describe('callbackifyClass', function () {
+  describe('basic functionality', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        async asyncAdd(b) {
+          return this.a + b
+        }
+      }
+      this.Callbackified = callbackifyClass(this.Class)
+    })
+
+    it('callbackifies the class methods', function (done) {
+      const adder = new this.Callbackified(1)
+      adder.asyncAdd(2, (err, sum) => {
+        expect(err).not.to.exist
+        expect(sum).to.equal(3)
+        done()
+      })
+    })
+  })
+
+  describe('without option', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        async asyncAdd(b) {
+          return this.a + b
+        }
+
+        syncAdd(b) {
+          return this.a + b
+        }
+      }
+      this.Callbackified = callbackifyClass(this.Class, {
+        without: ['syncAdd'],
+      })
+    })
+
+    it('does not callbackify excluded functions', function () {
+      const adder = new this.Callbackified(10)
+      const sum = adder.syncAdd(12)
+      expect(sum).to.equal(22)
+    })
+
+    it('callbackifies other functions', function (done) {
+      const adder = new this.Callbackified(1)
+      adder.asyncAdd(2, (err, sum) => {
+        expect(err).not.to.exist
+        expect(sum).to.equal(3)
+        done()
+      })
+    })
+  })
+
+  describe('multiResult option', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        async asyncAdd(b) {
+          return this.a + b
+        }
+
+        async asyncArithmetic(b) {
+          return { sum: this.a + b, product: this.a * b }
+        }
+      }
+      this.Callbackified = callbackifyClass(this.Class, {
+        multiResult: { asyncArithmetic: ['sum', 'product'] },
+      })
+    })
+
+    it('callbackifies multi-result functions', function (done) {
+      const adder = new this.Callbackified(3)
+      adder.asyncArithmetic(6, (err, sum, product) => {
+        expect(err).not.to.exist
+        expect(sum).to.equal(9)
+        expect(product).to.equal(18)
+        done()
+      })
+    })
+
+    it('callbackifies other functions normally', function (done) {
+      const adder = new this.Callbackified(6)
+      adder.asyncAdd(2, (err, sum) => {
+        expect(err).not.to.exist
+        expect(sum).to.equal(8)
+        done()
+      })
+    })
+  })
+})
+
 describe('expressify', function () {
   it('should propagate any rejection to the "next" callback', function (done) {
     const fn = () => Promise.reject(new Error('rejected'))

services/web/test/acceptance/src/helpers/Publisher.js

@@ -1,6 +1,6 @@
 const { ObjectId } = require('mongodb')
 const PublisherModel = require('../../../../app/src/models/Publisher').Publisher
-const { callbackify } = require('util')
+const { callbackifyClass } = require('@overleaf/promise-utils')
 
 let count = parseInt(Math.random() * 999999)
 
@@ -32,21 +32,7 @@ class PromisifiedPublisher {
   }
 }
 
-class Publisher extends PromisifiedPublisher {}
+const Publisher = callbackifyClass(PromisifiedPublisher)
 Publisher.promises = class extends PromisifiedPublisher {}
 
-// callbackify publisher class methods
-const nonPromiseMethods = ['constructor']
-Object.getOwnPropertyNames(PromisifiedPublisher.prototype).forEach(
-  methodName => {
-    const method = PromisifiedPublisher.prototype[methodName]
-    if (
-      typeof method === 'function' &&
-      !nonPromiseMethods.includes(methodName)
-    ) {
-      Publisher.prototype[methodName] = callbackify(method)
-    }
-  }
-)
-
 module.exports = Publisher

services/web/test/acceptance/src/helpers/Subscription.js

@@ -1,6 +1,6 @@
 const { db, ObjectId } = require('../../../../app/src/infrastructure/mongodb')
 const { expect } = require('chai')
-const { promisify } = require('util')
+const { promisifyClass } = require('@overleaf/promise-utils')
 const SubscriptionUpdater = require('../../../../app/src/Features/Subscription/SubscriptionUpdater')
 const PermissionsManager = require('../../../../app/src/Features/Authorization/PermissionsManager')
 const SSOConfigManager = require('../../../../modules/group-settings/app/src/sso/SSOConfigManager')
@@ -192,16 +192,8 @@ class Subscription {
   }
 }
 
-Subscription.promises = class extends Subscription {}
-
-// promisify User class methods - works for methods with 0-1 output parameters,
-// otherwise we will need to implement the method manually instead
-const nonPromiseMethods = ['constructor', 'getCapabilities']
-Object.getOwnPropertyNames(Subscription.prototype).forEach(methodName => {
-  const method = Subscription.prototype[methodName]
-  if (typeof method === 'function' && !nonPromiseMethods.includes(methodName)) {
-    Subscription.promises.prototype[methodName] = promisify(method)
-  }
+Subscription.promises = promisifyClass(Subscription, {
+  without: ['getCapabilities'],
 })
 
 Subscription.promises.prototype.inviteUser = async function (adminUser, email) {

services/web/test/acceptance/src/helpers/User.js

@@ -5,7 +5,7 @@ const { db, ObjectId } = require('../../../../app/src/infrastructure/mongodb')
 const UserModel = require('../../../../app/src/models/User').User
 const UserUpdater = require('../../../../app/src/Features/User/UserUpdater')
 const AuthenticationManager = require('../../../../app/src/Features/Authentication/AuthenticationManager')
-const { promisify } = require('util')
+const { promisifyClass } = require('@overleaf/promise-utils')
 const fs = require('fs')
 const Path = require('path')
 
@@ -1026,28 +1026,20 @@ class User {
   }
 }
 
-User.promises = class extends User {
-  doRequest(method, params) {
-    return new Promise((resolve, reject) => {
-      this.request[method.toLowerCase()](params, (err, response, body) => {
-        if (err) {
-          reject(err)
-        } else {
-          resolve({ response, body })
-        }
-      })
-    })
-  }
-}
-
-// promisify User class methods - works for methods with 0-1 output parameters,
-// otherwise we will need to implement the method manually instead
-const nonPromiseMethods = ['constructor', 'setExtraAttributes']
-Object.getOwnPropertyNames(User.prototype).forEach(methodName => {
-  const method = User.prototype[methodName]
-  if (typeof method === 'function' && !nonPromiseMethods.includes(methodName)) {
-    User.promises.prototype[methodName] = promisify(method)
-  }
+User.promises = promisifyClass(User, {
+  without: ['setExtraAttributes'],
 })
 
+User.promises.prototype.doRequest = async function (method, params) {
+  return new Promise((resolve, reject) => {
+    this.request[method.toLowerCase()](params, (err, response, body) => {
+      if (err) {
+        reject(err)
+      } else {
+        resolve({ response, body })
+      }
+    })
+  })
+}
+
 module.exports = User