search

amazon-archives / amazon-cognito-auth-js

The Amazon Cognito Auth SDK for JavaScript simplifies adding sign-up, sign-in with user profile functionality to web apps.
Apache License 2.0
424 stars 232 forks source link

Added callback argument to getSession() and parseCognitoWebResponse() #179

Closed mymattcarroll closed 10 months ago

mymattcarroll commented 5 years ago

Closes #114

Description of changes:

Added callback argument to getSession() and parseCognitoWebResponse() functions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

perrin4869 commented 5 years ago

+1, although I wonder, why do you not call this.userhandler.onSuccess if there is a callback? They aren't exclusive

mymattcarroll commented 5 years ago

@perrin4869 I only left it in there to be backward compatible to avoid a breaking change and requiring a new major version of the SDK.

IMO, the SDK should not support both ways going forward but only the callback argument passed to each function.

If this was my repository, I would have deleted the this.userhandler.onSuccess logic and made a new major version.

perrin4869 commented 5 years ago

Hm... In order to remain backwards compatible though, even if there is a callback, I think you should call the userhandler callbacks as well

mymattcarroll commented 5 years ago

When I say backwards compatible, I mean a developer could update the version of the SKD used in their application and it will continue to work as it did. Assuming this Pull Request was accepted and it was released with a minor version bump on NPM, if you update to that version, their application would continue to work without any changes. The changes suggested will achieve this.

I personally don't see a use case where a developer would want to handle authentication logic in two different places (I am not saying there is not one). IMO it is not adding flexibility, only complexity, and would lead to developers introducing race conditions by not knowing which one would run first. Developers would also have to make a decision on which method to use, which would require documentation on why the SDK supports two methods and suggestions on which one suits different scenarios. As stated above, this does not seem to add flexibility, only complexity (for no benefit IMO, again this is just my opinion and it may be wrong).

If the owners of the SDK agree with the your statements, I will change it so that both are supported but I would strongly suggest against that. The reason for this Pull Request is because the existing API of the SDK does not fulfil the needs required. If it did, I would not suggest two ways of getting a session and/or creating a session, I would just use the existing API.

ChristophP commented 4 years ago

Oh this has bitten me quite a bit. Also the documention is not clear about when onSuccess and onFailure are called. I changed some code. Now it is called twice. Very frustrating.

I completely agree with that paragraph.

I personally don't see a use case where a developer would want to handle authentication logic in two different places (I am not saying there is not one). IMO it is not adding flexibility, only complexity, and would lead to developers introducing race conditions by not knowing which one would run first. Developers would also have to make a decision on which method to use, which would require documentation on why the SDK supports two methods and suggestions on which one suits different scenarios. As stated above, this does not seem to add flexibility, only complexity (for no benefit IMO, again this is just my opinion and it may be wrong).

Also the docs could use a lot of work. The lib exposes about 10 classes but mentions only one or two of them. The Main Class CognitoAuth has only public members but most of those are most likely only meant for internal use. Very hard to follow. The docs should be improved and/or private method/fields should be made or at least marked as private.