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

Updated misconfigured dispatch #10511

Conversation

MatthewChon
Copy link
Contributor

Description of changes

Change verifytotptoken() to emit an 'verify' event instead of 'signIn'.
Added the case 'verify' to the listener.

Issue #, if available

#9750

Description of how you validated changes

i reproduced the issue and from there, I reran the application with the modified changes.
The hub now console logs 'verify' instead of 'signIn'.

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.

@MatthewChon MatthewChon requested a review from a team as a code owner October 20, 2022 20:25
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #10511 (84eace7) into main (54a122c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main   #10511   +/-   ##
=======================================
  Coverage   86.16%   86.16%           
=======================================
  Files         197      197           
  Lines       18369    18371    +2     
  Branches     3906     3907    +1     
=======================================
+ Hits        15827    15829    +2     
  Misses       2468     2468           
  Partials       74       74           
Impacted Files Coverage Δ
packages/auth/src/Auth.ts 87.38% <100.00%> (+0.02%) ⬆️

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

@abdallahshaban557 abdallahshaban557 added external-contributor Auth Related to Auth components/category Hub Related to Hub category labels Oct 22, 2022
@siegerts siegerts added the first-time-contributor The contribution is the first for this user in the repo label Oct 31, 2022
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

This change looks good to me. But I'm not sure whether stop emitting signIn is a breaking change for those expecting this event. I'd defer to other approvers.

@elorzafe
Copy link
Contributor

@MatthewChon

The only concern is that this could be a breaking change in case some one is listening for the signIn event. What if we emit both events until next major version change?

@MatthewChon
Copy link
Contributor Author

Ok, that sounds good!

@abdallahshaban557 abdallahshaban557 added the V6 V6 of Amplify JS label Dec 1, 2022
elorzafe
elorzafe previously approved these changes Dec 15, 2022
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.

LGTM thanks @MatthewChon 🎖️

AllanZhengYP
AllanZhengYP previously approved these changes Dec 15, 2022
Copy link
Contributor

@haverchuck haverchuck left a comment

Choose a reason for hiding this comment

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

@MatthewChon Please see inline comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category external-contributor first-time-contributor The contribution is the first for this user in the repo Hub Related to Hub category V6 V6 of Amplify JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants