-
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
React sign out #191
React sign out #191
Conversation
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
- Coverage 90.97% 89.67% -1.31%
==========================================
Files 59 59
Lines 2882 2964 +82
Branches 572 587 +15
==========================================
+ Hits 2622 2658 +36
- Misses 246 292 +46
Partials 14 14
Continue to review full report at Codecov.
|
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.
Looks ok, proposing a few changes / updates.
@@ -46,11 +48,51 @@ export default class Greetings extends AuthPiece { | |||
} | |||
|
|||
signOut() { | |||
this.googleSignOut(); |
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.
We do some sort of check here first before calling these so we don't run into unexpected exceptions with the specific frameworks. Perhaps we should abstract this out into the Auth.signOut() method as well, so the error returned in catch() could be related to federated.
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.
Check is implemented in the beginning of those methods.
Auth.signOut() | ||
.then(() => this.changeState('signedOut')) | ||
.catch(err => { logger.error(err); this.error(err); }); | ||
} | ||
|
||
googleSignOut() { | ||
const auth2 = window.gapi && window.gapi.auth2? window.gapi.auth2 : null; |
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.
update here to not include int in the string name, use something like authGoogle etc.
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.
auth2 is a name given by Google.
} | ||
|
||
facebookSignOut() { | ||
const fb = window.FB; |
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.
check if window.FB exists first
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.
Will be checked in the next line.
…plify into react-sign-out
@@ -33,7 +34,7 @@ export default function withGoogle(Comp) { | |||
}; | |||
|
|||
const { onStateChange } = this.props; | |||
return Auth.federatedSignIn('google', { token: id_token, expires_at }, user) | |||
return Auth.federatedSignIn('google', { token: id_token, expires_at, refreshing: false }, user) | |||
.then(crednetials => { |
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.
typo 'crednetials ' = credentials
@@ -46,6 +46,7 @@ export default function withFacebook(Comp) { | |||
if (!accessToken) { | |||
return; | |||
} | |||
const that = this; |
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.
remove
@@ -32,6 +32,7 @@ export default function withGoogle(Comp) { | |||
name: profile.getName() | |||
}; | |||
|
|||
const that = this; |
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.
remove
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.
👍
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
For issue #180
Refreshing federated jwt token on need.