From e04f0c877ba94c126bbf94622640700c728f74c7 Mon Sep 17 00:00:00 2001
From: kumavis <aaron@kumavis.me>
Date: Fri, 29 Mar 2019 12:37:40 +0800
Subject: [PATCH] lib - nodeify - correctly wrap synchronous functions

---
 app/scripts/lib/nodeify.js    | 15 ++++++++---
 test/unit/app/nodeify-test.js | 49 ++++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js
index 25be6537b..a813ae679 100644
--- a/app/scripts/lib/nodeify.js
+++ b/app/scripts/lib/nodeify.js
@@ -1,5 +1,5 @@
 const promiseToCallback = require('promise-to-callback')
-const noop = function () {}
+const callbackNoop = function (err) { if (err) throw err }
 
 /**
  * A generator that returns a function which, when passed a promise, can treat that promise as a node style callback.
@@ -11,6 +11,7 @@ const noop = function () {}
  */
 module.exports = function nodeify (fn, context) {
   return function () {
+    // parse arguments
     const args = [].slice.call(arguments)
     const lastArg = args[args.length - 1]
     const lastArgIsCallback = typeof lastArg === 'function'
@@ -19,8 +20,16 @@ module.exports = function nodeify (fn, context) {
       callback = lastArg
       args.pop()
     } else {
-      callback = noop
+      callback = callbackNoop
     }
-    promiseToCallback(fn.apply(context, args))(callback)
+    // call the provided function and ensure result is a promise
+    let result
+    try {
+      result = Promise.resolve(fn.apply(context, args))
+    } catch (err) {
+      result = Promise.reject(err)
+    }
+    // wire up promise resolution to callback
+    promiseToCallback(result)(callback)
   }
 }
diff --git a/test/unit/app/nodeify-test.js b/test/unit/app/nodeify-test.js
index 938b76c68..0dafc6653 100644
--- a/test/unit/app/nodeify-test.js
+++ b/test/unit/app/nodeify-test.js
@@ -2,16 +2,16 @@ const assert = require('assert')
 const nodeify = require('../../../app/scripts/lib/nodeify')
 
 describe('nodeify', function () {
-  var obj = {
+  const obj = {
     foo: 'bar',
     promiseFunc: function (a) {
-      var solution = this.foo + a
+      const solution = this.foo + a
       return Promise.resolve(solution)
     },
   }
 
   it('should retain original context', function (done) {
-    var nodified = nodeify(obj.promiseFunc, obj)
+    const nodified = nodeify(obj.promiseFunc, obj)
     nodified('baz', function (err, res) {
       if (!err) {
         assert.equal(res, 'barbaz')
@@ -22,7 +22,7 @@ describe('nodeify', function () {
     })
   })
 
-  it('should allow the last argument to not be a function', function (done) {
+  it('no callback - should allow the last argument to not be a function', function (done) {
     const nodified = nodeify(obj.promiseFunc, obj)
     try {
       nodified('baz')
@@ -31,4 +31,45 @@ describe('nodeify', function () {
       done(new Error('should not have thrown if the last argument is not a function'))
     }
   })
+
+  it('no callback - should asyncly throw an error if underlying function does', function (done) {
+    const nodified = nodeify(async () => { throw new Error('boom!') }, obj)
+    process.prependOnceListener('uncaughtException', function (err) {
+      assert.ok(err, 'got expected error')
+      assert.ok(err.message.includes('boom!'), 'got expected error message')
+      done()
+    })
+    try {
+      nodified('baz')
+    } catch (err) {
+      done(new Error('should not have thrown an error synchronously'))      
+    }
+  })
+
+  it('sync functions - returns value', function (done) {
+    const nodified = nodeify(() => 42)
+    try {
+      nodified((err, result) => {
+        if (err) return done(new Error(`should not have thrown any error: ${err.message}`))
+        assert.equal(42, result, 'got expected result')
+      })
+      done()
+    } catch (err) {
+      done(new Error(`should not have thrown any error: ${err.message}`))
+    }
+  })
+
+  it('sync functions - handles errors', function (done) {
+    const nodified = nodeify(() => { throw new Error('boom!') })
+    try {
+      nodified((err, result) => {
+        if (result) return done(new Error('should not have returned any result'))
+        assert.ok(err, 'got expected error')
+        assert.ok(err.message.includes('boom!'), 'got expected error message')
+      })
+      done()
+    } catch (err) {
+      done(new Error(`should not have thrown any error: ${err.message}`))
+    }
+  })
 })