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

Added clearSession iOS Safari Method #65

Merged
merged 7 commits into from
Aug 18, 2017
Merged

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Jul 20, 2017

Added clearSession JS wrapper

@cocojoe
Copy link
Member Author

cocojoe commented Jul 20, 2017

Also ran it through Xcode/Re-Indent 😄
I didn't make the SFView semi-transparent as the more I use it, I feel by showing the browser it shows more clearly to the user that it's doing something.

@cocojoe
Copy link
Member Author

cocojoe commented Jul 20, 2017

Note: Sample PR auth0-samples/auth0-react-native-sample#4

@cocojoe cocojoe requested a review from hzalaz July 31, 2017 12:27
webauth/index.js Outdated
clearSession(options = {}, callback) {
const { clientId, domain, client, agent } = this;
const federated = options.federated || false;
A0Auth0.clearSession(domain, federated, callback);
Copy link
Member

Choose a reason for hiding this comment

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

Since this method is not yet available on android lets fail when the OS is not iOS.

Copy link
Member

Choose a reason for hiding this comment

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

webauth/index.js Outdated
*
* @memberof WebAuth
*/
clearSession(options = {}, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

The library uses promises instead of callbacks

Added clearSession JS wrapper
Added iOS platform check to method
@cocojoe cocojoe force-pushed the added-clearsession-safari branch from 5fb74c3 to 4c6cb1a Compare August 14, 2017 10:38
@cocojoe cocojoe added this to the v1-Next milestone Aug 16, 2017
@cocojoe
Copy link
Member Author

cocojoe commented Aug 16, 2017

Ping

ios/A0Auth0.m Outdated
self.sessionCallback = callback;
}

RCT_EXPORT_METHOD(clearSession:(NSString *)domain federated:(BOOL)federated callback:(RCTResponseSenderBlock)callback) {
NSString *urlString = [NSString stringWithFormat:@"https://%@/v2/logout", domain];
Copy link
Member

Choose a reason for hiding this comment

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

Lets use NSURLComponents to build urls

Copy link
Member

Choose a reason for hiding this comment

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

And TBH I'd rather build the full url in JS and make this method a more generic one like show and close on load thing

ios/A0Auth0.m Outdated
[self terminateWithError:error dismissing:NO animated:NO];
}

- (void)safariViewController:(SFSafariViewController *)controller didCompleteInitialLoad:(BOOL)didLoadSuccessfully {
if (!didLoadSuccessfully) {
if (self.didLoadCallback) {
didLoadSuccessfully ? self.didLoadCallback(@[[NSNull null]]) : self.didLoadCallback(@[@{@"error": @"failed to load"}]);
Copy link
Member

Choose a reason for hiding this comment

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

Probably cleaner to do the ternary operator with the parameter and the call the callback later

ios/A0Auth0.m Outdated
};
@"error": @"a0.session.failed_load",
@"error_description": @"Failed to load authorize url"
};
[self terminateWithError:error dismissing:YES animated:YES];
Copy link
Member

Choose a reason for hiding this comment

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

We need to clean the didLoadCallback too

Copy link
Member Author

Choose a reason for hiding this comment

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

What are you thinking here, refactor it all to use the didLoadCallback and make the auto close optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore that, out of scope of this PR.

webauth/index.js Outdated
return new Promise(function (resolve, reject) {
if (Platform.OS !== 'ios') { reject(new Error('clearSession only supported in iOS')); }
A0Auth0.clearSession(domain, federated, function (err, data) {
if (err !== null) return reject(err);
Copy link
Member

Choose a reason for hiding this comment

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

Always use {}

hzalaz
hzalaz previously approved these changes Aug 18, 2017
Make use of Auth client url builder.
Make iOS implementation reuse existing handlers
@hzalaz hzalaz force-pushed the added-clearsession-safari branch from 7f0edb0 to 493ed73 Compare August 18, 2017 02:17
@hzalaz hzalaz merged commit f48d415 into master Aug 18, 2017
@hzalaz hzalaz deleted the added-clearsession-safari branch August 18, 2017 03:03
@hzalaz hzalaz modified the milestones: v1-Next, v1.1.0 Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants