feat: Load .mjs files as scripts (#1657)

BREAKING CHANGE: This change introduces using async/await to load files from the scripts folder.

bin/e2e-test.sh

@@ -15,6 +15,9 @@ cat <<EOF > external-scripts.json
 ["hubot-diagnostics"]
 EOF
 
+mkdir -p $TEMP_ROOT/scripts
+cp $HUBOT_FOLDER/test/fixtures/TestScript.mjs $TEMP_ROOT/scripts/
+
 # npm install /path/to/hubot will create a symlink in npm 5+ (http://blog.npmjs.org/post/161081169345/v500).
 # As the require calls for app-specific scripts happen inside hubot, we have to
 # set NODE_PATH to the app’s node_modules path so they can be found

bin/hubot.js

@@ -11,7 +11,7 @@ const switches = [
   ['-a', '--adapter ADAPTER', 'The Adapter to use, e.g. "shell" (to load the default hubot shell adapter)'],
   ['-f', '--file PATH', 'Path to adapter file, e.g. "./adapters/CustomAdapter.mjs"'],
   ['-c', '--create PATH', 'Create a deployable hubot'],
-  ['-d', '--disable-httpd', 'Disable the HTTP server'],
+  ['-d', '--disable-httpd DISABLE_HTTPD', 'Disable the HTTP server'],
   ['-h', '--help', 'Display the help information'],
   ['-l', '--alias ALIAS', "Enable replacing the robot's name with alias"],
   ['-n', '--name NAME', 'The name of the robot in chat'],
@@ -98,21 +98,23 @@ if (options.file) {
   options.adapter = options.file.split('/').pop().split('.')[0]
 }
 const robot = Hubot.loadBot(options.adapter, options.enableHttpd, options.name, options.alias)
+module.exports = robot
 
-function loadScripts () {
-  robot.load(pathResolve('.', 'scripts'))
-  robot.load(pathResolve('.', 'src', 'scripts'))
+async function loadScripts () {
+  await robot.load(pathResolve('.', 'scripts'))
+  await robot.load(pathResolve('.', 'src', 'scripts'))
 
   loadExternalScripts()
 
-  options.scripts.forEach((scriptPath) => {
+  const tasks = options.scripts.map((scriptPath) => {
     console.log('loadding', scriptPath)
     if (scriptPath[0] === '/') {
       return robot.load(scriptPath)
     }
 
-    robot.load(pathResolve('.', scriptPath))
+    return robot.load(pathResolve('.', scriptPath))
   })
+  await Promise.all(tasks)
 }
 
 function loadExternalScripts () {
@@ -144,7 +146,7 @@ function loadExternalScripts () {
   }
 
   if (options.configCheck) {
-    loadScripts()
+    await loadScripts()
     console.log('OK')
     process.exit(0)
   }

src/robot.js

@@ -346,30 +346,47 @@ class Robot {
     })
   }
 
+  async loadmjs (filePath) {
+    const script = await import(filePath)
+    if (typeof script?.default === 'function') {
+      script.default(this)
+    } else {
+      this.logger.warning(`Expected ${filePath} to assign a function to export default, got ${typeof script}`)
+    }
+  }
+
+  async loadcoffee (filePath) {
+    return await this.loadjs(filePath)
+  }
+
+  async loadjs (filePath) {
+    const script = require(filePath)
+    if (typeof script === 'function') {
+      script(this)
+    } else {
+      this.logger.warning(`Expected ${filePath} to assign a function to module.exports, got ${typeof script}`)
+    }
+  }
+
   // Public: Loads a file in path.
   //
   // filepath - A String path on the filesystem.
   // filename - A String filename in path on the filesystem.
   //
   // Returns nothing.
-  loadFile (filepath, filename) {
-    const ext = path.extname(filename)
-    const full = path.join(filepath, path.basename(filename, ext))
+  async loadFile (filepath, filename) {
+    const ext = path.extname(filename)?.replace('.', '')
+    const full = path.join(filepath, path.basename(filename))
 
     // see https://github.com/hubotio/hubot/issues/1355
-    if (['.js', '.mjs', '.coffee'].indexOf(ext) == -1) { // eslint-disable-line
+    if (['js', 'mjs', 'coffee'].indexOf(ext) === -1) {
+      this.logger.debug(`Skipping unsupported file type ${full}`)
       return
     }
 
     try {
-      const script = require(full)
-
-      if (typeof script === 'function') {
-        script(this)
-        this.parseHelp(path.join(filepath, filename))
-      } else {
-        this.logger.warning(`Expected ${full} to assign a function to module.exports, got ${typeof script}`)
-      }
+      await this[`load${ext}`](full)
+      this.parseHelp(full)
     } catch (error) {
       this.logger.error(`Unable to load ${full}: ${error.stack}`)
       process.exit(1)
@@ -381,11 +398,12 @@ class Robot {
   // path - A String path on the filesystem.
   //
   // Returns nothing.
-  load (path) {
+  async load (path) {
     this.logger.debug(`Loading scripts from ${path}`)
 
     if (fs.existsSync(path)) {
-      fs.readdirSync(path).sort().map(file => this.loadFile(path, file))
+      const tasks = fs.readdirSync(path).sort().map(file => this.loadFile(path, file))
+      await Promise.all(tasks)
     }
   }
 

test/es2015_test.js

@@ -195,5 +195,6 @@ describe('hubot/es2015', function () {
     expect(loadBot).to.be.a('function')
     Hubot.loadBot('adapter', 'enableHttpd', 'botName', 'botAlias')
     expect(Hubot.Robot).to.be.called.calledWith('adapter', 'enableHttpd', 'botName', 'botAlias')
+    sinon.restore()
   })
 })

test/fixtures/MockAdapter.mjs

@@ -9,19 +9,19 @@ class MockAdapter extends Adapter {
   }
 
   send (envelope, ...strings) {
-    this.emit('send', envelope, strings)
+    this.emit('send', envelope, ...strings)
   }
 
   reply (envelope, ...strings) {
-    this.emit('reply', envelope, strings)
+    this.emit('reply', envelope, ...strings)
   }
 
   topic (envelope, ...strings) {
-    this.emit('topic', envelope, strings)
+    this.emit('topic', envelope, ...strings)
   }
 
   play (envelope, ...strings) {
-    this.emit('play', envelope, strings)
+    this.emit('play', envelope, ...strings)
   }
 
   run () {

test/fixtures/TestScript.js

@@ -5,8 +5,8 @@
 // Commands:
 //   hubot test - Responds with a test response
 //
-
 module.exports = robot => {
+  robot.hasLoadedTestJsScript = true
   robot.respond('test', res => {
     res.send('test response')
   })

test/fixtures/TestScript.mjs

@@ -0,0 +1,14 @@
+'use strict'
+
+// Description: A test .mjs script for the robot to load
+//
+// Commands:
+//   hubot test mjs - Responds with a test response from a .mjs script
+//
+
+export default robot => {
+  robot.hasLoadedTestMjsScript = true
+  robot.respond(/test$/, res => {
+    res.reply('test response from .mjs script')
+  })
+}

test/fixtures/TestScriptIncorrectApi.js

@@ -0,0 +1,9 @@
+'use strict'
+
+// Description: A test script for the robot to load
+//
+// Commands:
+//   hubot test - Responds with a test response
+//
+
+module.exports = {}

test/fixtures/TestScriptIncorrectApi.mjs

@@ -0,0 +1,9 @@
+'use strict'
+
+// Description: A test .mjs script for the robot to load
+//
+// Commands:
+//   hubot test mjs - Responds with a test response from a .mjs script
+//
+
+export default {}

test/hubot_test.js

@@ -0,0 +1,34 @@
+'use strict'
+
+/* global describe, it, before, after */
+/* eslint-disable no-unused-expressions */
+
+const path = require('path')
+const chai = require('chai')
+chai.use(require('sinon-chai'))
+const expect = chai.expect
+const root = __dirname.replace(/test$/, '')
+const { TextMessage, User } = require('../index.js')
+
+describe('hubot', () => {
+  let hubot
+  before(() => {
+    process.env.HUBOT_ADAPTER = path.join(__dirname, './fixtures/MockAdapter.mjs')
+    hubot = require('../bin/hubot.js')
+  })
+  after(() => {
+    hubot.shutdown()
+    delete process.env.HUBOT_ADAPTER
+  })
+  it('should export robot instance', done => {
+    hubot.loadFile(path.resolve(root, 'test/fixtures'), 'TestScript.mjs').then(() => {
+      hubot.adapter.on('reply', (envelope, ...strings) => {
+        expect(strings[0]).to.equal('test response from .mjs script')
+        done()
+      })
+      hubot.receive(new TextMessage(new User('mocha', { room: '#mocha' }), 'Hubot test'))
+      expect(hubot.hasLoadedTestMjsScript).to.be.true
+      expect(hubot.name).to.equal('Hubot')
+    }).catch(done)
+  })
+})

test/robot_test.js

@@ -382,71 +382,42 @@ describe('Robot', function () {
         this.sandbox.restore()
       })
 
-      it('should require the specified file', function () {
-        const module = require('module')
-
-        const script = sinon.spy(function (robot) {})
-        this.sandbox.stub(module, '_load').returns(script)
-        this.sandbox.stub(this.robot, 'parseHelp')
+      it('should require the specified file', async function () {
+        await this.robot.loadFile(path.resolve('./test/fixtures'), 'TestScript.js')
+        expect(this.robot.hasLoadedTestJsScript).to.be.true
+      })
 
-        this.robot.loadFile('./scripts', 'TestScript.js')
-        expect(module._load).to.have.been.calledWith(path.join('scripts', 'TestScript'))
+      it('should load an .mjs file', async function () {
+        await this.robot.loadFile(path.resolve('./test/fixtures'), 'TestScript.mjs')
+        expect(this.robot.hasLoadedTestMjsScript).to.be.true
       })
 
       describe('proper script', function () {
-        beforeEach(function () {
-          const module = require('module')
-
-          this.script = sinon.spy(function (robot) {})
-          this.sandbox.stub(module, '_load').returns(this.script)
-          this.sandbox.stub(this.robot, 'parseHelp')
-        })
-
-        it('should call the script with the Robot', function () {
-          this.robot.loadFile('./scripts', 'TestScript.js')
-          expect(this.script).to.have.been.calledWith(this.robot)
-        })
-
-        it('should parse the script documentation', function () {
-          this.robot.loadFile('./scripts', 'TestScript.js')
-          expect(this.robot.parseHelp).to.have.been.calledWith(path.join('scripts', 'TestScript.js'))
+        it('should parse the script documentation', async function () {
+          await this.robot.loadFile(path.resolve('./test/fixtures'), 'TestScript.js')
+          expect(this.robot.helpCommands()).to.eql(['hubot test - Responds with a test response'])
         })
       })
 
       describe('non-Function script', function () {
-        beforeEach(function () {
-          const module = require('module')
-
-          this.script = {}
-          this.sandbox.stub(module, '_load').returns(this.script)
-          this.sandbox.stub(this.robot, 'parseHelp')
-        })
-
-        it('logs a warning for a .js file', function () {
+        it('logs a warning for a .js file that does not export the correct API', async function () {
           sinon.stub(this.robot.logger, 'warning')
-          this.robot.loadFile('./scripts', 'TestScript.js')
+          await this.robot.loadFile(path.resolve('./test/fixtures'), 'TestScriptIncorrectApi.js')
           expect(this.robot.logger.warning).to.have.been.called
         })
 
-        it('logs a warning for a .mjs file', function () {
+        it('logs a warning for a .mjs file that does not export the correct API', async function () {
           sinon.stub(this.robot.logger, 'warning')
-          this.robot.loadFile('./scripts', 'TestScript.mjs')
+          await this.robot.loadFile(path.resolve('./test/fixtures'), 'TestScriptIncorrectApi.mjs')
           expect(this.robot.logger.warning).to.have.been.called
         })
       })
 
       describe('unsupported file extension', function () {
-        beforeEach(function () {
-          const module = require('module')
-
-          this.script = sinon.spy(function (robot) {})
-          this.sandbox.stub(module, '_load').returns(this.script)
-          this.sandbox.stub(this.robot, 'parseHelp')
-        })
-
-        it('should not be loaded by the Robot', function () {
-          this.robot.loadFile('./scripts', 'unsupported.yml')
-          expect(this.script).to.not.have.been.calledWith(this.robot)
+        it('should not be loaded by the Robot', async function () {
+          sinon.spy(this.robot.logger, 'debug')
+          await this.robot.loadFile(path.resolve('./test/fixtures'), 'unsupported.yml')
+          expect(this.robot.logger.debug).to.have.been.calledWithMatch(/unsupported file type/)
         })
       })
     })
@@ -1107,7 +1078,7 @@ describe('Robot ES6', () => {
     robot = new Robot('MockAdapter', true, 'TestHubot')
     robot.alias = 'Hubot'
     await robot.loadAdapter('./test/fixtures/MockAdapter.mjs')
-    robot.loadFile(path.resolve('./test/fixtures/'), 'TestScript.js')
+    await robot.loadFile(path.resolve('./test/fixtures/'), 'TestScript.js')
     robot.run()
   })
   afterEach(() => {
@@ -1134,7 +1105,7 @@ describe('Robot Coffeescript', () => {
     robot = new Robot('MockAdapter', true, 'TestHubot')
     robot.alias = 'Hubot'
     await robot.loadAdapter('./test/fixtures/MockAdapter.coffee')
-    robot.loadFile(path.resolve('./test/fixtures/'), 'TestScript.coffee')
+    await robot.loadFile(path.resolve('./test/fixtures/'), 'TestScript.coffee')
     robot.run()
   })
   afterEach(() => {