Closed simonbuchan closed 6 years ago
👍 for Promises.
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.
@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 :)
In fact, AWS SDK 2.3 added Promises supprt! (sort-of) https://aws.amazon.com/releasenotes/SDK/JavaScript/8589740860839559
I will bring this up with the team so that we integrate it in our future roadmap.
@itrestian So, what is the future roadmap?
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.
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.
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
.
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();
}
Definitely interested in anything that helps developers since developers seem to wrap calls in promises anyway https://github.com/aws/amazon-cognito-identity-js/issues/79
Backwards compatibility as you pointed out is a requirement, we don't want people to change their code.
@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: https://github.com/aws/aws-sdk-js/issues/2#issuecomment-11113355
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.
@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.
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.
I'm writing tests for the SDK here, once I've got good coverage I (and AWS!) can be more confident that doing this won't break usage.
The big part will be CognitoUser.authenticateUser()
: it's a very complex beefy method that is literally built to check there is a real implementation on the other side that isn't just returning static data, so I might have to essentially have the test implement a real SRP server, but hopefully I can just stub out random().
All the rest looks like they could be done pretty quick.
How are you getting on Simon? I'd like to be able to use Promises for this library too, but dont have enough experience to tackle the translation myself at the moment.
@FatherDougal I want to finish getting test coverage in, since this would be more of a rewrite than the ES6 port I did for webpack in #108, but I'm making progress on that, see #154
It has been quite some time since this was updated, is there a status update?
+1 for Promises, would it be possible to fork a WIP branch?
Any news on the promise based function ? AWS.DynamoDB.DocumentClient() can return promises so aws cognito should be able to do the same. Any ETA ?
Is promise supported yet?
?
Hey, any news :) ?
Any news on this? I'm also interested in using Promises for this API.
These async node callbacks are a mess. I just spent all afternoon wondering why this happens:
console.log('1')
this.user.getUserAttributes((err: Error, attributes: Array<CognitoUserAttribute>) => {
console.log('3')
}
console.log('2')
There is nothing anywhere to indicate that this is an async callback. For somebody who is new to JavaScript and doesn't know about NodeCallback this is a pain. I've just wrapped it in a promise but it would be nice by default.
The docs or examples could be a bit more enlightening at the very least...
I'm having the same concerns... every way of coding these promises in seemingly the correct manner, gives me all kinds of grief, with the main one being 'Callback is not a function.' :( What can we do here... I'm assuming most of my users will use the provided Login with Facebook, or Login with Google, and save me a tonne of trouble authenticating through the signUp, verify, process... but I still need it for the odd user who doesn't have FB or Google, but it seems that AWS doesn't like people writing new code.
You can have a look at AWS Amplify which exposes a higher order auth component for this SDK. Maybe that can suit your use case. Also note that we will continue developing this library in the AWS Amplify repo.
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:With callback object styles, I currently only propose making
onSuccess
andonFailure
optional, other callback properties can still make sense (but probably should be broken into multiple calls later):OLD PRE-ES6 CODE Strawman implementations (untested)
A minimal (hand-written) diff for hybrid node style and Promise returning:
Simply wrapping with
polygoat
:Rewriting using AWS SDK promise support and hybrid-izer wrapper: