Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return types of functions in AuthClass are unsafe #6053

Closed
bklebe opened this issue Jun 10, 2020 · 6 comments
Closed

Return types of functions in AuthClass are unsafe #6053

bklebe opened this issue Jun 10, 2020 · 6 comments
Labels
Auth Related to Auth components/category feature-request Request a new feature TypeScript Related to TypeScript issues

Comments

@bklebe
Copy link

bklebe commented Jun 10, 2020

I want to start by saying that I write this issue without a solid understanding of the history of the project and its philosophy of TypeScript usage, and with as much an intention of learning what guarantees I should expect from this library so I can work with it more effectively as it is, as suggesting that they be made stronger.

Describe the bug
Auth.currentAuthenticatedUser(), Auth.currentUserPoolUser() and many others return Promise<CognitoUser | any> or another type that is a union with a specific type and any. This means that these functions in practice return Promise<any> and not Promise<CognitoUser>. Am I to expect that I can't assume anything about their return value, or should I be using a type assertion as CognitoUser? Leaving aside that CognitoUser does not appear to fully describe what is returned by these functions, as identified by #4927, this usage of union types means using these functions is not type safe.

To Reproduce
Call any function (i.e. "foo()") on the result of Auth.currentAuthenticatedUser().

Expected behavior
These functions should return T instead of T | any; accessing properties that do not exist on their return types should fail to typecheck.

Code Snippet

Auth.currentAuthenticatedUser()
  .then((user) => user.foo())
  .catch((error) => console.log(error)); // Prints "user.foo is not a function"
@bklebe bklebe added the to-be-reproduced Used in order for Amplify to reproduce said issue label Jun 10, 2020
@amhinson
Copy link
Contributor

@beeuhtricks I think the reason the return type is CognitoUser | any is partly due to the fact that when using Identity Pool Federation, user could essentially be anything that a provider returns as a user, which won't be the same as CognitoUser.

Reference: https://docs.amplify.aws/lib/auth/advanced/q/platform/js#identity-pool-federation

Note that this isn't from a Cognito User Pool so the user you get after calling this method is not a Cognito User.

@amhinson amhinson added Auth Related to Auth components/category question General question TypeScript Related to TypeScript issues and removed to-be-reproduced Used in order for Amplify to reproduce said issue question General question labels Jun 11, 2020
@bklebe
Copy link
Author

bklebe commented Jun 11, 2020

@beeuhtricks I think the reason the return type is CognitoUser | any is partly due to the fact that when using Identity Pool Federation, user could essentially be anything that a provider returns as a user, which won't be the same as CognitoUser.

This makes sense, however there are ways to express this without totally obliterating the other type in the union, for example CognitoUser | { payload: any }, and then we could write a type guard for CognitoUser that would allow us to branch safely on the result, yes?

@elorzafe elorzafe added this to the Auth v2 milestone Jun 11, 2020
@sammartinez sammartinez added the feature-request Request a new feature label Jul 6, 2020
@sammartinez
Copy link
Contributor

@bklebe Thanks for this feedback to us, I have marked this as a feature request as we have work slated later this year to improve this.

@jimblanc
Copy link
Contributor

We have published an RFC on our plan for improving TypeScript support in Amplify JS & would love to get your feedback & suggestions!

RFC: Amplify JS TypeScript Improvements

@cwomack
Copy link
Member

cwomack commented Oct 3, 2023

The developer preview for v6 of Amplify has officially been released with improved support for TypeScript and much more! Please check out our announcement and updated documentation to see what has changed.

This issue should be resolved within the dev preview and upcoming General Availability for Amplify v6, but let us know with a comment if there are further issues.

@cwomack
Copy link
Member

cwomack commented Nov 16, 2023

With the release of the latest major version of Amplify (aws-amplify@>6), this issue should now be resolved! Please refer to our release announcement, migration guide, and documentation for more information.

@cwomack cwomack closed this as completed Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category feature-request Request a new feature TypeScript Related to TypeScript issues
Projects
None yet
Development

No branches or pull requests

7 participants