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

React sign out #191

Merged
merged 32 commits into from
Mar 19, 2018
Merged

React sign out #191

merged 32 commits into from
Mar 19, 2018

Conversation

powerful23
Copy link
Contributor

@powerful23 powerful23 commented Jan 25, 2018

For issue #180

Refreshing federated jwt token on need.

@codecov-io
Copy link

codecov-io commented Jan 25, 2018

Codecov Report

Merging #191 into master will decrease coverage by 1.3%.
The diff coverage is 47.87%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/aws-amplify-react/src/Auth/Greetings.jsx 95.58% <100%> (+9.22%) ⬆️
packages/aws-amplify/src/Auth/Auth.ts 81.72% <30%> (-7.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ad2ce6...927327d. Read the comment docs.

Copy link
Contributor

@mlabieniec mlabieniec left a 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();
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

@@ -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 => {
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor

@richardzcode richardzcode left a comment

Choose a reason for hiding this comment

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

👍

@richardzcode richardzcode merged commit b439e08 into aws-amplify:master Mar 19, 2018
@powerful23 powerful23 deleted the react-sign-out branch July 3, 2018 23:45
@github-actions
Copy link

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 *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants