-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: main
Are you sure you want to change the base?
feat: custom user agent CognitoUser changes for UI handoff #11643
Conversation
There was a problem hiding this 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.
packages/amazon-cognito-identity-js/src/internals/InternalCognitoUserPool.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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).
Codecov Report
❗ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this 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.
.circleci/config.yml
Outdated
@@ -1632,6 +1632,7 @@ releasable_branches: &releasable_branches | |||
- release | |||
- main | |||
- next | |||
- feat/custom-user-agent-ui/cognito |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Description of changes
NOTES
Description of how you validated changes
yarn tests pass
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.