Async APIs should be returning Promises

[simonbuchan]

The current callbacks are a mess of node-style and callback objects, and are often immediately invoked, a problem for re-entrancy. I propose all callback accepting methods are updated to return a Promise and to make the callback argument optional.

With all the current lib dependencies, adding a dependency for global Promise or polyfill should not be a big deal.

There is a growing expectation that async methods return a Promise, they compose far better, and the proposed (and available now with babel) async / await language feature make them as simple to use as synchronous methods:

// callback style, still works
function getUserEmailCallback(cognitoUser, callback) {
    cognitoUser.getUserAttributes(function(err, result) {
        if (err) {
            return callback(err, null);
        }
        var attr = result.filter(function (x) { return x.getName() === 'email'; })[0];
        if (!attr) {
            return callback(new Error('User missing "email" attribute?'), null);
        }
        callback(null, attr.getValue());
    });
}

// promise style
function getUserEmailPromise(cognitoUser) {
    return cognitoUser.getUserAttributes().then(function (result) {
        var attr = result.filter(function (x) { return x.getName() === 'email'; })[0];
        if (!attr) {
            throw new Error('User missing "email" attribute?');
        }
        return attr.getValue();
    });
}

// promise style used with async function language feature
async function getUserEmailAsync(cognitoUser) {
    var result = await cognitoUser.getUserAttributes();
    var attr = result.filter(function (x) { return x.getName() === 'email'; })[0];
    if (!attr) {
        throw new Error('User missing "email" attribute?');
    }
    return attr.getValue();
}

With callback object styles, I currently only propose making onSuccess and onFailure optional, other callback properties can still make sense (but probably should be broken into multiple calls later):

function provideEmailVerificationCallbackl(cognitoUser, verificationCode, callback) {
    cognitoUser.getAttributeVerificationCode('email', {
        onSuccess: function (result) {
            callback(null, result);
        },
        onFailure: function(err) {
            callback(err, null);
        },
        inputVerificationCode() {
            cognitoUser.verifyAttribute('email', verificationCode, this);
        }
    });
}

function provideEmailVerificationPromise(cognitoUser, verificationCode) {
    return cognitoUser.getAttributeVerificationCode('email', {
        inputVerificationCode() {
            cognitoUser.verifyAttribute('email', verificationCode, this);
        }
    });
}

OLD PRE-ES6 CODE Strawman implementations (untested)

A minimal (hand-written) diff for hybrid node style and Promise returning:

     CognitoUser.prototype.deleteUser = function deleteUser(callback) {
+        var promise = new Promise(function (resolve, reject) {
         if (this.signInUserSession != null && this.signInUserSession.isValid()) {
             this.client.deleteUser({
                 AccessToken : this.signInUserSession.getAccessToken().getJwtToken()
             }, function (err, data) {
                 if (err) {
-                    return callback(err, null);
+                    return reject(err);
                 } else {
-                    return callback(null, 'SUCCESS');
+                    return resolve('SUCCESS');
                 }
             });
         } else {
-            return callback(new Error('User is not authenticated'), null);
+            throw new Error('User is not authenticated');
         }
+        });
+        if (callback) {
+            promise.then(function (data) { callback(null, data); }, function (err) { callback(err, null); });
+        }
+        return promise;
     };

Simply wrapping with polygoat:

-    CognitoUser.prototype.deleteUser = function deleteUser(callback) {
+    CognitoUser.prototype.deleteUser = function deleteUser(userCallback) {
+        return polygoat(function (callback) {
         if (this.signInUserSession != null && this.signInUserSession.isValid()) {
             this.client.deleteUser({
                 AccessToken : this.signInUserSession.getAccessToken().getJwtToken()
             }, function (err, data) {
                 if (err) {
                     return callback(err, null);
                 } else {
                     return callback(null, 'SUCCESS');
                 }
             });
         } else {
             return callback(new Error('User is not authenticated'), null);
         }
+        }, userCallback);
     };

Rewriting using AWS SDK promise support and hybrid-izer wrapper:

    CognitoUser.prototype.deleteUser = toNodePromiseHybrid(function deleteUser() {
        if (this.signInUserSession != null && this.signInUserSession.isValid()) {
            return this.client.deleteUser({
                AccessToken : this.signInUserSession.getAccessToken().getJwtToken()
            }).promise();
        } else {
            throw new Error('User is not authenticated');
        }
    });

    function toNodePromiseHybrid(method) {
        return function () {
            var args = Array.prototype.slice(arguments);
            var callback = null;
            if (args.length && args[args.length - 1] instanceof Function) {
                callback = args.pop();
            }
            var promise = method.apply(this, args);
            if (callback) {
                promise.then(function (data) { callback(null, data); }, function (err) { callback(err, null); });
            }
            return promise;
        };
    }

[RickDT]

👍 for Promises.

[ribomation]

I agree that a promise based API would be neat.
However, the AWS SDK for JS is based on the function(err,data) {...} callback style.
Look at any method in http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/
e.g. http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/DynamoDB.html#listTables-property

When that have been said; I agree there are a lot of room for improvements of the cognito-identity JS library before it gets finalized and merged into the AWS-SDK-JS code base.

[simonbuchan]

@ribomation I take it your issue is with the mismatch in style, not implementation issues? I don't think this library is intended to be merged into the AWS SDK? Amazon provide plenty of support SDKs that sit on top of the raw SDK, but those may be temporary until merged too, for all I know.

Ideally, of course, the same change would be made to the base SDK, then there's no problem :)

[simonbuchan]

In fact, AWS SDK 2.3 added Promises supprt! (sort-of) https://aws.amazon.com/releasenotes/SDK/JavaScript/8589740860839559

[itrestian]

I will bring this up with the team so that we integrate it in our future roadmap.

[ribomation]

@itrestian So, what is the future roadmap?

• Is this library going to be merged into the AWS SDK for JS and when will it happen?
• Are there any new updates to this library in the pipeline? AFAIK the last code update was done in early May.
• Which changes and/or features are planned?
• Are you AWS guys driving the code base forward or are you sort of relying on the community to finalize the library?

I guess many developers, me included, is wondering about the status of this project; if and when we can start putting it into production code.

[chetanme]

The library was updated last week the General Availability launch of the feature. We will keep updating the library with more features down the road. Community contributions are also welcome and this should be fit for production use.

It will stay as opt in library for now. Currently, we do not have any plans to merge this with AWS SDK.

[simonbuchan]

Would you guys be interested in a PR for this? The main issue is the Promise (of some sort) dependency and increase of code complexity.

The first can be handled by piggy-backing on aws-sdk's promise support, the latter ideally would be using async / await, but that currerntly would bloat the built code a bit (babel-transform-async-functions piggy-backs on ES6 generators + regenerator).

With example usage picked to show off options:

this.client.makeUnauthenticatedRequest('foo', { ... }, (err, fooRes) => {
  if (err) {
    return callback(err, null); // or return callback.onFailure(err);
  }
  this.client.makeUnauthenticatedRequest('bar', { ..., useFoo(fooRes) }, (err, barRes) => {
    if (err) {
      return callback(err, null); // or return callback.onFailure(err);
    }
    this.client.makeUnauthenticatedRequest('baz', { ..., useFooAndBar(fooRes, barRes) }, (err, bazRes) => {
    callback(null, useBazRes(bazRes)); // or return callback.onSuccess();
  });
});

Using just .then() might be best:

return handleCallback(callback, this.client.makeUnauthenticatedRequest('foo', { ... }).promise()
.then(fooRes =>
  this.client.makeUnauthenticatedRequest('bar', { ..., useFoo(fooRes) }).promise()
  .then(barRes =>
    this.client.makeUnauthenticatedRequest('baz', { ..., useFooAndBar(fooRes, barRes) }).promise()
  )
)
.then(bazRes => useBaz(bazRes));

Where handleCallback does the right thing:

function handleCallback(callback, promise) {
  if (callback) {
    if (typeof callback === 'function') {
      return promise.then(
        res => { callback(null, res); },
        err => { callback(err, null); }
      );
    }
    if (typeof callback === 'object' && callback.onSuccess && callback.onFailure) {
      return promise.then(
        res => { callback.onSuccess(res); },
        err => { callback.onFailure(err); }
      );
    }
    throw new Error('Bad callback');
  }
  return promise;
}

Note that also unifies callbacks so users don't need to care which each method takes.

The main problem would be supporting things like callback.mfaRequired() when that doesn't need to call back.

Probably easiest to have it simply reject with a well defined reason:

user.authenticateUser(..., {
  onSuccess() { showLoggedIn(user) },
  onFailure(err) { showError(err) },
  mfaRequired() { showMFA() }, // should not call onSuccess
});

Becomes:

try {
  session = await user.authenticateUser(...);
} catch (err) {
  if (err === CognitoUser.MFA_REQUIRED_ERROR) { // or CognitoError.isMFARequired(err)?
    showMFA();
  } else {
    showError(err);
  }
}

Though that is a bit ugly. Maybe resolve with a status code if we can (eg it should also resolve the callback)

The other wart is authenticateUser() may pass a 2nd parameter to onSuccess(): userConfirmationNecessary.

[paulsson]

Promises please! I'd love to be able to use them like I do with the rest of aws-sdk-js:

Sign-up page action handlers calling my "service" that handles all user related auth:

onConfirm(form) {
  let promise = this.userData.confirmSignUp(this.confirm.username, this.confirm.code);
  promise.then(
    (data) => {
      this.handleConfirmSuccess(data);
    },
    (err) => {
      console.log("err = " + err);
      if(err.code == "ExpiredCodeException") {
        this.handleExpiredCode();
      }
    }
  );
}

UserData (service / provider) class

confirmSignUp(username: string, confirmationCode: string) {
  var params = {
    ClientId: AppConfig.COGNITO_CLIENT_ID,
    Username: username,
    ConfirmationCode: confirmationCode
  };
  return this.cognitoIdentityServiceProvdider.confirmSignUp(params).promise();
}

or

signup(username: string, email: string, password: string) {
  let params = {
    ClientId: AppConfig.COGNITO_CLIENT_ID,
    Username: username,
    Password: password,
    UserAttributes: [
      { Name: 'email', Value: email }
    ]
  };
  return this.cognitoIdentityServiceProvdider.signUp(params).promise();
}

[itrestian]

Definitely interested in anything that helps developers since developers seem to wrap calls in promises anyway #79

Backwards compatibility as you pointed out is a requirement, we don't want people to change their code.

[paulsson]

@itrestian I just wanted to pass along a couple links regarding promises and keeping backwards compatibility as related to what the aws-sdk-js did to accomplish this.

First please check out the conversation for this github issue for aws-sdk-js where this was discussed and the beginnings for promises:
aws/aws-sdk-js#2 (comment)

Here's an AWS blog post covering this:
https://blogs.aws.amazon.com/javascript/post/Tx3BZ2DC4XARUGG/Support-for-Promises-in-the-SDK

Basically, what they did was if a callback function is not provided as the last arg then the function returns an AWSRequest object which has a .promise() function available.

[simonbuchan]

@paulsson The reason they had to do it that way was they were already returning an AWS.Request object, even without a callpack parameter (in which case it was not yet started and you had to call send() on it), so it would break back-compat to return a promise directly.
This library doesn't have that issue, none of the callback-taking methods return a value, so it should use the conventional returning an in-progress promise.

[paulsson]

Thanks for the explanation @simonbuchan. Just seems like consistency among the SDK would be desired. I'm sure whatever is implemented will be a good fit with the rest of the SDK.