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

Wrong type definitions on user.signOut() #11109

Closed
3 tasks done
seivan opened this issue Mar 21, 2023 · 6 comments
Closed
3 tasks done

Wrong type definitions on user.signOut() #11109

seivan opened this issue Mar 21, 2023 · 6 comments
Labels
feature-request Request a new feature TypeScript Related to TypeScript issues

Comments

@seivan
Copy link

seivan commented Mar 21, 2023

Before opening, please confirm:

JavaScript Framework

Not applicable

Amplify APIs

Authentication

Amplify Categories

auth

Environment information

# Put output below this line

Irrellevant 

Describe the bug

The signature is incorrect for signOut since it passes an error in the callback, but missing in type definition

public signOut(callback?: () => void): void;

Expected behavior

The signature for signOut should pass an optional error in the callback

public signOut(callback?: (error?: Error | undefined) => void): void;

Reproduction steps

user.signOut(() => {
//? Handle error? 
}

Code Snippet

// Put your code below this line.

Log output

No response

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

@seivan seivan added the pending-triage Issue is pending triage label Mar 21, 2023
@tannerabread tannerabread added the TypeScript Related to TypeScript issues label Mar 21, 2023
@tannerabread
Copy link
Contributor

Hi @seivan we are working on improvements to our types and TypeScript so I will bring this up with the rest of the team to ensure it gets included in the efforts.

Thanks for bringing it to our attention!

@seivan
Copy link
Author

seivan commented Mar 21, 2023

Do you want me to fix it? Or rewrite the cognito layer in TS? I don’t mind and would probably fix most of your issues.

@tannerabread
Copy link
Contributor

Hi @seivan if you would like to fix the signOut function I think we can get that shipped pretty quickly.

As I mentioned there is a large effort to rewrite the TS in the library in the works, so I would ask that you don't expend the extra effort to rewrite the entire cognito layer in TS.

@seivan
Copy link
Author

seivan commented Mar 21, 2023

Hi @seivan if you would like to fix the signOut function I think we can get that shipped pretty quickly.

Sure!

As I mentioned there is a large effort to rewrite the TS in the library in the works, so I would ask that you don't expend the extra effort to rewrite the entire cognito layer in TS.

Is there an open Roadmap and feedback pipeline?

I mean the default API leaves a lot to wish for, not just types but better ergonomics. Say something as simple as getting the email attribute would look like this in you wanna track the happy path while still retaining error context.

  public getEmail = async (): Promise<string> => {
    const context = errorContextCreator("Validate Session")
    return Promise
      .resolve(this._cognitoUser)
      .then(u => {
        if (u == null) { throw context("no user") }
        else { return u }
      })
      .then(user => {
        return new Promise((resolve, reject) => {
          user.getUserData((errorData, valueData) => {
            Promise
              .resolve({errorData, valueData})
              .then(response => {
                if (response.errorData != null) { throw context("get user data failed", response.errorData) }
                else { return response.valueData }
              })
              .then(data => {
                if (data == null) { throw context("get user data returned empty") }
                else { return data.UserAttributes }
              })
              .then(attributes => {
                return attributes.find(a => {
                  return a.Name.toLowerCase() === "email"
                })?.Value
              })
              .then(email => {
                if (email == null || email == "") { throw context("no email found") }
                else { return email }
              })
              .then(email => {
              // this._cognitoEmailAttribute = email
                resolve(email)
              })
              .catch(e => {
                reject(e)
              })



          })
        })
      })
  }

@cwomack cwomack removed the pending-triage Issue is pending triage label Mar 21, 2023
@tannerabread
Copy link
Contributor

Hi @seivan I was waiting for it to go live but you can check out this new RFC for typescript improvements and leave feedback there so that we can hopefully incorporate all of your feedback.

@jimblanc jimblanc removed their assignment Mar 23, 2023
@jimblanc jimblanc added V6 V6 of Amplify JS and removed V6 V6 of Amplify JS labels Mar 23, 2023
@stocaaro stocaaro added the investigating This issue is being investigated label Apr 13, 2023
@tannerabread tannerabread added feature-request Request a new feature and removed investigating This issue is being investigated labels Apr 25, 2023
@cwomack
Copy link
Contributor

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
feature-request Request a new feature TypeScript Related to TypeScript issues
Projects
None yet
Development

No branches or pull requests

5 participants