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

feat: custom user agent CognitoUser changes for UI handoff #11643

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

erinleigh90
Copy link
Member

Description of changes

  • Creates and exports InternalCognitoUser class from /internals scope with added customUserAgentDetails parameter on public API's that result in service calls
  • Sends received customUserAgentDetails through to storage service calls
  • Send user agent information from Auth to CognitoUser

NOTES

  • To see the changes to InternalCognitoUser after moving the changes over from CognitoUser, see this comparison

Description of how you validated changes

yarn tests pass

Checklist

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

@erinleigh90 erinleigh90 requested review from a team as code owners July 18, 2023 04:22
@erinleigh90 erinleigh90 requested a review from a team as a code owner July 18, 2023 18:37
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Overall looks good @erinleigh90

My only concern is InternalCognitoUserPool is not extended by the external one.

Copy link
Member

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Same concern as Francisco around the CognitoUserPool extension, but mostly looks good. It might be a good idea to run the integ tests against this branch just due to the volume of changes (too late now, but --no-verify would be useful here to avoid all the prettier changes).

@erinleigh90 erinleigh90 requested a review from a team as a code owner July 19, 2023 07:54
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2023

Codecov Report

Merging #11643 (444ef9b) into main (7365c34) will increase coverage by 0.25%.
The diff coverage is 87.41%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main   #11643      +/-   ##
==========================================
+ Coverage   83.63%   83.89%   +0.25%     
==========================================
  Files         341      342       +1     
  Lines       21581    20915     -666     
  Branches     4611     4455     -156     
==========================================
- Hits        18050    17547     -503     
+ Misses       3244     3105     -139     
+ Partials      287      263      -24     
Impacted Files Coverage Δ
packages/amazon-cognito-identity-js/src/Client.js 50.94% <64.28%> (+0.45%) ⬆️
...ages/amazon-cognito-identity-js/src/CognitoUser.js 92.40% <84.61%> (+13.34%) ⬆️
packages/auth/src/Auth.ts 87.84% <87.19%> (+0.25%) ⬆️
packages/auth/src/utils.ts 100.00% <100.00%> (ø)
packages/core/src/Platform/types.ts 100.00% <100.00%> (ø)

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

Copy link
Member

@israx israx left a comment

Choose a reason for hiding this comment

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

User agent changes look good. I really wanna see this metrics.

My only concern is about not changing the public facing API types. Although those changes are not breaking. Proposed return values don't align with return values for v6. I might've missed some APIs but the feedback is for all public API types that are being changed by this PR.

packages/auth/src/Auth.ts Outdated Show resolved Hide resolved
packages/auth/src/Auth.ts Outdated Show resolved Hide resolved
packages/auth/src/Auth.ts Outdated Show resolved Hide resolved
packages/auth/src/Auth.ts Outdated Show resolved Hide resolved
@@ -1632,6 +1632,7 @@ releasable_branches: &releasable_branches
- release
- main
- next
- feat/custom-user-agent-ui/cognito
Copy link
Member

Choose a reason for hiding this comment

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

Hum appears this didn't trigger a run in circle CI. Probably need to push feature branch to main repo (can keep the PR branch how it is).

@erinleigh90 erinleigh90 requested a review from israx July 19, 2023 17:13
Copy link
Member Author

Choose a reason for hiding this comment

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

Go back through and update jsdocs

@erinleigh90 erinleigh90 marked this pull request as draft July 26, 2023 06:34
stocaaro
stocaaro previously approved these changes Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants