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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
612a43d
fix(auth): Missing type in configure function
micksatana Jun 30, 2022
fced710
docs: Update config type for configure function
micksatana Jun 30, 2022
692d520
Merge branch 'main' into auth/fix-missing-configure-type
chintannp Jul 20, 2022
f542470
Merge branch 'main' into auth/fix-missing-configure-type
micksatana Aug 2, 2022
a0b5cc9
Merge branch 'main' into auth/fix-missing-configure-type
micksatana Sep 1, 2022
e96061e
Merge branch 'main' into auth/fix-missing-configure-type
stocaaro Dec 1, 2022
771985b
Merge branch 'main' into auth/fix-missing-configure-type
stocaaro Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/api/classes/authclass.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,7 +1287,7 @@ <h4 class="tsd-returns-title">Returns <span class="tsd-signature-type">Promise</
<a name="configure" class="tsd-anchor"></a>
<h3>configure</h3>
<ul class="tsd-signatures tsd-kind-method tsd-parent-kind-class">
<li class="tsd-signature tsd-kind-icon">configure<span class="tsd-signature-symbol">(</span>config<span class="tsd-signature-symbol">?: </span><span class="tsd-signature-type">any</span><span class="tsd-signature-symbol">)</span><span class="tsd-signature-symbol">: </span><a href="../interfaces/authoptions.html" class="tsd-signature-type">AuthOptions</a></li>
<li class="tsd-signature tsd-kind-icon">configure<span class="tsd-signature-symbol">(</span>config<span class="tsd-signature-symbol">?: </span><span class="tsd-signature-type">AuthOptions</span><span class="tsd-signature-symbol">)</span><span class="tsd-signature-symbol">: </span><a href="../interfaces/authoptions.html" class="tsd-signature-type">AuthOptions</a></li>
</ul>
<ul class="tsd-descriptions">
<li class="tsd-description">
Expand Down Expand Up @@ -2709,4 +2709,4 @@ <h2>Legend</h2>
gtag('config', 'UA-115615468-1');
</script>
</body>
</html>
</html>
2 changes: 1 addition & 1 deletion packages/auth/src/Auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited

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.

if (!config) return this._config || {};
logger.debug('configure Auth');
const conf = Object.assign(
Expand Down
1 change: 1 addition & 0 deletions packages/auth/src/types/Auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export interface AuthOptions {
identityPoolRegion?: string;
clientMetadata?: any;
endpoint?: string;
ssr?: boolean;
}

export enum CognitoHostedUIIdentityProvider {
Expand Down