Fix bug in callback-to-promise with varying argument lengths (#103)

This was a latent bug that was exposed after some testing, but is not new.

We use an internal function, `callbackToPromise`, to allow functions to
be in "callback mode" or "promise mode":

    myTable.create({foo: 'boo'}, (err, record) => {
        // ...
    });

    const record = myTable.create({foo: 'boo'});

When calling `callbackToPromise`, you can specify an optional
`callbackArgIndex`. If it's unspecified (which it usually is), the
callback index is assumed to be the last argument.

However, it had a bug. The logic I stated above, "the callback index is
assumed to be the last argument", wasn't done every time, but only the
_first_ time. That meant that something like this would fail:

    function sum(a, b, maybeC, callback) {
        let c;
        if (callback === undefined) {
            callback = maybeC;
            c = 0;
        } else {
            c = maybeC;
        }

        callback(null, a + b + c);
    }

    const wrapped = callbackToPromise(sum, null);

    // In the following call, the callback arg index is the 3rd argument,
    // which is correct.
    wrapped(1, 2, () => {
        // In the following call, the callback arg index is STILL the 3rd
        // argument, which is 5--that's incorrect. This would then return
        // a Promise, which is unexpected, and never call the callback.
        wrapped(3, 4, 5, () => {
            // This is never called!
        });
    });

This wasn't just theoretical. This hangs forever on `[email protected]`
(make sure to fill in real values):

    const myTable = new Airtable({apiKey: 'key123'})
        .base('app123')
        .table('My Table');

    myTable.create({foo: 'bar'}, (err1) => {
        if (err1) { throw err1; }

        myTable.create({foo: 'baz'}, {typecast: true}, (err2) => {
            // The record is created but this callback is never called
            // because the second call to `myTable.create` returned a
            // Promise.
        });
    });

I've made sure to add tests for `callbackToPromise` as part of this commit.

lib/callback_to_promise.js

@@ -8,19 +8,21 @@
  */
 function callbackToPromise(fn, context, callbackArgIndex) {
     return function() {
-        // If callbackArgIndex isn't provided, use the last argument.
+        var thisCallbackArgIndex;
         if (callbackArgIndex === void 0) {
-            callbackArgIndex = arguments.length > 0 ? arguments.length - 1 : 0;
+            thisCallbackArgIndex = arguments.length > 0 ? arguments.length - 1 : 0;
+        } else {
+            thisCallbackArgIndex = callbackArgIndex;
         }
-        var callbackArg = arguments[callbackArgIndex];
+        var callbackArg = arguments[thisCallbackArgIndex];
         if (typeof callbackArg === 'function') {
             fn.apply(context, arguments);
         } else {
             var args = [];
             // If an explicit callbackArgIndex is set, but the function is called
             // with too few arguments, we want to push undefined onto args so that
             // our constructed callback ends up at the right index.
-            var argLen = Math.max(arguments.length, callbackArgIndex);
+            var argLen = Math.max(arguments.length, thisCallbackArgIndex);
             for (var i = 0; i < argLen; i++) {
                 args.push(arguments[i]);
             }

test/callback_to_promise.test.js

@@ -0,0 +1,50 @@
+'use strict';
+
+var callbackToPromise = require('../lib/callback_to_promise');
+
+describe('callbackToPromise', function () {
+    function returnThisPlusValue(value, callback) {
+        callback(null, this + value); // jshint ignore:line
+    }
+
+    function sum() {
+        var callback = arguments[arguments.length - 1];
+        var result = 0;
+        for (var i = 0; i < arguments.length - 1; i++) {
+            result += arguments[i];
+        }
+        callback(null, result);
+    }
+
+    it('lets a function return a promise', function () {
+        var wrapped = callbackToPromise(returnThisPlusValue, 1);
+        expect(wrapped(2)).resolves.toBe(3);
+    });
+
+    it('maintains the ability to call a function with a callback', function (done) {
+        var wrapped = callbackToPromise(returnThisPlusValue, 1);
+        wrapped(2, function (err, result) {
+            expect(err).toBeNull();
+            expect(result).toBe(3);
+            done();
+        });
+    });
+
+    it('is resilient to changes in the number of arguments', function (done) {
+        var wrapped = callbackToPromise(sum, null);
+
+        wrapped(1, 2, function (err1, result1) {
+            expect(err1).toBeNull();
+            expect(result1).toBe(3);
+            wrapped(3, 4, 5, function (err2, result2) {
+                expect(err2).toBeNull();
+                expect(result2).toBe(12);
+                wrapped(6, function (err3, result3) {
+                    expect(err3).toBeNull();
+                    expect(result3).toBe(6);
+                    done();
+                });
+            });
+        });
+    });
+});