search

aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.42k stars 2.12k forks source link

Return types of functions in AuthClass are unsafe #6053

Closed bklebe closed 10 months ago

bklebe commented 4 years ago

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"
amhinson commented 4 years ago

@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.

bklebe commented 4 years ago

@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?

sammartinez commented 4 years ago

@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 commented 1 year ago

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 commented 11 months ago

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 commented 10 months ago

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.