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

fix(auth): Missing type in configure function #10045

Closed

Conversation

micksatana
Copy link
Contributor

@micksatana micksatana commented Jun 30, 2022

Description of changes

Screen Shot 2565-06-30 at 23 35 40

Missing type when using `Auth.configure`

Description of how you validated changes

Screen Shot 2565-07-01 at 00 28 20

In VSCode

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

decebal
decebal previously approved these changes Jul 1, 2022
@micksatana micksatana force-pushed the auth/fix-missing-configure-type branch 2 times, most recently from 6de5928 to e0ae056 Compare July 2, 2022 02:25
@siegerts siegerts added the first-time-contributor The contribution is the first for this user in the repo label Jul 5, 2022
@micksatana micksatana force-pushed the auth/fix-missing-configure-type branch from e0ae056 to 78328c7 Compare July 12, 2022 02:12
@micksatana micksatana force-pushed the auth/fix-missing-configure-type branch from 78328c7 to fced710 Compare July 16, 2022 09:28
@micksatana
Copy link
Contributor Author

Hi guys, can we have one more approver here? It's a simple change, won't take long to review.

@chintannp
Copy link
Contributor

@micksatana, Thanks for your contribution and also updating the docs at the same time.

chintannp
chintannp previously approved these changes Jul 20, 2022
Copy link
Contributor

@chintannp chintannp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@chrisbonifacio chrisbonifacio requested review from a team and removed request for nickarocho July 21, 2022 15:13
@chrisbonifacio
Copy link
Contributor

chrisbonifacio commented Jul 21, 2022

@micksatana
Looks like we need to fix one of the tests to account for the added type

 Test suite failed to run
@aws-amplify/auth:     TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
@aws-amplify/auth:     __tests__/auth-configure-test.ts:16:19 - error TS2345: Argument of type '{ userPoolId: string; userPoolWebClientId: string; region: string; identityPoolId: string; mandatorySignIn: boolean; storage: {}; }' is not assignable to parameter of type 'AuthOptions'.
@aws-amplify/auth:       Types of property 'storage' are incompatible.
@aws-amplify/auth:         Type '{}' is missing the following properties from type 'ICognitoStorage': setItem, getItem, removeItem, clear
@aws-amplify/auth:     16  		auth.configure(opts);

@@ -139,7 +139,7 @@ export class AuthClass {
return 'Auth';
}

configure(config?) {
configure(config?: AuthOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change unfortunately. Auth.configure can also use the content of aws-exports.js file. Maybe we could do something to overload this function? @jamesaucode what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chrisbonifacio

Type '{}' is missing the following properties from type 'ICognitoStorage' because the test case use {} instead of just left it out when storage not exists. The test mentioning throw error when storage is empty, so it should throw and it does. When I ran test locally, it got

@aws-amplify/auth: PASS __tests__/auth-configure-test.ts
@aws-amplify/auth:   ● Console
@aws-amplify/auth:     console.error ../core/lib/Logger/ConsoleLogger.js:129
@aws-amplify/auth:       [ERROR] 40:02.742 AuthClass - The storage in the Auth config is not valid!

And

@aws-amplify/auth: Test Suites: 11 passed, 11 total
@aws-amplify/auth: Tests:       200 passed, 200 total
@aws-amplify/auth: Snapshots:   1 passed, 1 total
@aws-amplify/auth: Time:        14.282s

I'm not sure how the same test behave differently in CI though.

Copy link
Contributor Author

@micksatana micksatana Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test case want to test the fail case when it's an empty object. But it conflicts with type testing as the ICognitoStorage type has all required properties. So to pass this test, either fix the test case to remove storage: {} out, or make all ICognitoStorage props optional. If the test case intent to test logic not type, I think we shouldn't test with empty object (without ts-ignore). It should test with undefined instead. Or keep the empty object storage but use ts-ignore above the auth.configure(opts); line because we intentionally make the type failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @micksatana - Just wanted to give you a heads up we are planning on pushing a bunch of changes to our Authentication category, and are planning on include changes in your PR as a part of that. Just wanted to reach out to make sure you know we did not abandon your contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdallahshaban557 acknowledged with thanks.

@stocaaro stocaaro added the feature-request Request a new feature label Mar 23, 2023
@jimblanc jimblanc removed the V6 V6 of Amplify JS label Mar 23, 2023
@abdallahshaban557
Copy link
Contributor

Hi @micksatana - thank you for this contribution! We are currently working on improving our TS types, including adding support for typing the configure function. You can check out our RFC proposal here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor feature-request Request a new feature first-time-contributor The contribution is the first for this user in the repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants