From 9ef084d73fca41dfdd1738b356b3204371c6b8a3 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Fri, 8 Mar 2024 09:27:18 +0000 Subject: [PATCH] Merge pull request #17430 from overleaf/dp-callbackify-class Add callbackifyClass utility GitOrigin-RevId: 762b800ce0eff2f146147908838162f7d32bd855 --- libraries/promise-utils/index.js | 31 +++++- .../test/unit/PromiseUtilsTests.js | 103 ++++++++++++++++++ .../test/acceptance/src/helpers/Publisher.js | 18 +-- .../acceptance/src/helpers/Subscription.js | 14 +-- .../web/test/acceptance/src/helpers/User.js | 38 +++---- 5 files changed, 153 insertions(+), 51 deletions(-) diff --git a/libraries/promise-utils/index.js b/libraries/promise-utils/index.js index 5b81a43e14..6e40e4c2f3 100644 --- a/libraries/promise-utils/index.js +++ b/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) diff --git a/libraries/promise-utils/test/unit/PromiseUtilsTests.js b/libraries/promise-utils/test/unit/PromiseUtilsTests.js index 39304cdc44..ad0e12a634 100644 --- a/libraries/promise-utils/test/unit/PromiseUtilsTests.js +++ b/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')) diff --git a/services/web/test/acceptance/src/helpers/Publisher.js b/services/web/test/acceptance/src/helpers/Publisher.js index a631db6f0d..12e94662d7 100644 --- a/services/web/test/acceptance/src/helpers/Publisher.js +++ b/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 diff --git a/services/web/test/acceptance/src/helpers/Subscription.js b/services/web/test/acceptance/src/helpers/Subscription.js index 1edc730b75..80854db037 100644 --- a/services/web/test/acceptance/src/helpers/Subscription.js +++ b/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) { diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index b028a0784b..d281471799 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/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