refactor(amazon-cognito-identity-js): refactor issue 8969 better typings #9935

Yoshiitaka · 2022-05-25T11:28:54Z

Issue ##8969,

Better type definitions for amazon-cognito-identity-js.

Checklist

PR description included

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

…typings

ShawnCockburn · 2022-06-30T13:13:52Z

+1

chintannp · 2022-07-21T15:19:34Z

Hey @Yoshiitaka ,
Thanks for your contribution to the AWS Amplify JS Library. We are actively reviewing your PR and will provide feedback soon if needed.

chintannp · 2022-07-21T17:21:44Z

@Yoshiitaka ,
I won't be able fix the merge conflict as your forked branch is behind. Can you fix the merge conflicts so that we can review the PR and be able to merge it?

Yoshiitaka · 2022-07-21T23:36:06Z

@chintannp
Thank you check this PR.
I updated the forked branch and fixed the conflicts part :)
Please Could you review this PR?

elorzafe

@Yoshiitaka thanks for this PR,

I have some concerns with typing some information that is available for getters, Another big concern is that attributes and token payload can be configured on the UserPool and could break the type.

What do you think?

elorzafe · 2022-07-27T19:18:56Z

packages/amazon-cognito-identity-js/index.d.ts

+		payload: {
+			sub: string;
+			email_verified: boolean;
+			iss: string;
+			phone_number_verified: boolean;
+			'cognito:username': string;
+			given_name: string;
+			origin_jti: string;
+			aud: string;
+			event_id: string;
+			token_use: string;
+			auth_time: number;
+			phone_number: string;
+			exp: number;
+			iat: number;
+			family_name: string;
+			jti: string;
+			email: string;


A problem here is that this payload can modified ona userpool lambda trigger

hmm...
In other words, does this place have to be as follows, not a strict type?

payload: { [key: string]: any };

elorzafe · 2022-07-27T19:22:17Z

packages/amazon-cognito-identity-js/index.d.ts


+	export interface CognitoAccessToken {
+		jwtToken: string;
+		payload: {


A the problem here is that decodePayload return type is { [id: string]: any }

hmm..
In other words, does this place have to be as follows, not a strict type too?

payload: { [key: string]: any };

Interesting, the interface could be more restricted. I need to verify if those claims are same across every userpool

elorzafe · 2022-07-27T19:24:02Z

packages/amazon-cognito-identity-js/index.d.ts

+		attributes: {
+			email: string;
+			email_verified: boolean;
+			family_name: string;
+			given_name: string;
+			phone_number: string;
+			phone_number_verified: boolean;
+			sub: string;
+		};
+


those got lazily load, and after that those are available, the other problem is that a user can have more attributes

In that case, I began think that it is better not to create strict type information for Cognito User attributes.

I agree, do you want to revert that change?

Thank you for comment. yes, revert this changes.

elorzafe · 2022-07-27T19:25:04Z

packages/amazon-cognito-identity-js/index.d.ts

+		public clockDrift: number;
+		public idToken: CognitoIdToken;
+		public accessToken: CognitoAccessToken;
+		public refreshToken: CognitoRefreshToken;
+


why do you need to expose this values if there are already getters?

I'm see... That is as you pointed out.
there are already created the getter,
there was no need to expose public values.
Thank you :)

Do you want to revert that change?

yes, revert this changes too.

chintannp · 2022-07-29T18:30:24Z

@Yoshiitaka, In order to get this change out quickly, Can you please review and address the feedback provided by @elorzafe ?

Yoshiitaka · 2022-07-30T12:18:56Z

@elorzafe
Thank you for reviewing.
This PR was dealt with a long time ago, So I will check the points to be pointed out and corrections from now.

Certainly, I was understood that the token payload may break the mold as you pointed out.

Yoshiitaka · 2022-07-30T12:19:08Z

@chintannp
Sorry for the late confirmation and reply.
I will check the review pointed out from this and correct it.

…typings

chintannp · 2022-08-22T18:10:33Z

@Yoshiitaka , Can you address the feedback provide by @elorzafe ?

Yoshiitaka · 2022-08-23T03:20:10Z

@chintannp
I have already responded to @elorzafe 's feedback comment, So Please could you check it?

…typings

codecov-commenter · 2022-08-23T03:39:07Z

Codecov Report

Merging #9935 (e463eda) into main (91fdcd9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #9935   +/-   ##
=======================================
  Coverage   84.40%   84.40%           
=======================================
  Files         258      258           
  Lines       18698    18698           
  Branches     4021     4021           
=======================================
  Hits        15782    15782           
  Misses       2826     2826           
  Partials       90       90

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…typings

aminya · 2023-01-18T08:50:46Z

When I use Hub.listen and get the payload.data, the found CognitoUser doesn't have any methods. That's because it is serialized. So, the properties should be still added. The getters are not enough.

yo-koitabashi and others added 3 commits May 11, 2022 20:24

refactor better typings for auth

53ed866

fix payload type

c55b9f5

Merge branch 'aws-amplify:main' into auth/refactor-issue-8969-better-…

85ffb95

…typings

siegerts added the first-time-contributor The contribution is the first for this user in the repo label May 25, 2022

chrisbonifacio requested review from ashika01, elorzafe and jamesaucode July 11, 2022 17:54

Fix confrict code

33c4e6d

elorzafe reviewed Jul 27, 2022

View reviewed changes

Merge branch 'aws-amplify:main' into auth/refactor-issue-8969-better-…

2a16a76

…typings

siegerts added the external-contributor label Aug 16, 2022

Merge branch 'aws-amplify:main' into auth/refactor-issue-8969-better-…

bb2084e

…typings

Yoshiitaka and others added 2 commits September 1, 2022 00:50

Merge branch 'aws-amplify:main' into auth/refactor-issue-8969-better-…

66cb4cf

…typings

Revert changes

e463eda

manueliglesias requested a review from a team as a code owner September 30, 2022 21:21

manueliglesias force-pushed the main branch from 5b68474 to 66bfe31 Compare September 30, 2022 21:48

aminya approved these changes Jan 18, 2023

View reviewed changes

cwomack added the amazon-cognito-identity-js Used for issues related to this specific package within the monorepo label Feb 20, 2024

refactor(amazon-cognito-identity-js): refactor issue 8969 better typings #9935

Are you sure you want to change the base?

refactor(amazon-cognito-identity-js): refactor issue 8969 better typings #9935

Uh oh!

Conversation

Yoshiitaka commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue ##8969,

Checklist

Uh oh!

ShawnCockburn commented Jun 30, 2022

Uh oh!

chintannp commented Jul 21, 2022

Uh oh!

chintannp commented Jul 21, 2022

Uh oh!

Yoshiitaka commented Jul 21, 2022

Uh oh!

elorzafe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yoshiitaka Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chintannp commented Jul 29, 2022

Uh oh!

Yoshiitaka commented Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yoshiitaka commented Jul 30, 2022

Uh oh!

chintannp commented Aug 22, 2022

Uh oh!

Yoshiitaka commented Aug 23, 2022

Uh oh!

codecov-commenter commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aminya commented Jan 18, 2023

Uh oh!

Uh oh!

Yoshiitaka commented May 25, 2022 •

edited

Loading

Yoshiitaka Aug 31, 2022 •

edited

Loading

Yoshiitaka commented Jul 30, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading