Skip to content

Conversation

@hakonk
Copy link

@hakonk hakonk commented Jun 26, 2018

I tried running pod install and ran into an error due to missing url for s.homepage :)

@kadikraman
Copy link
Contributor

Hey, thanks for the PR! Could you change the url to point to the GitHub repo (https://github.com/FormidableLabs/react-native-app-auth) instead? We don't have a landing page for this library in our main website, so that would make more sense.

@hakonk
Copy link
Author

hakonk commented Jun 26, 2018

Sure!

I came across some other stuff as well that I’d like to add to the PR. Will push more code tomorrow

hakonk added 3 commits June 27, 2018 09:59
…ession and calling on id instance conforming to UIApplicationDelegate in order to get reference to rootViewController
@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

@kadikraman Here are some additional changes :)

  • It seems the source code path was incorrectly set.

  • Regarding retrieving a reference to the rootViewController that should present the web view instance or the SFAuthenticationViewController instance, I've proposed calling on an id instance which conforms to UIApplicationDelegate instead.

  • Also, in order to decouple the this lib from the app, I propose we introduce a protocol RNAppAuthAuthorizationFlowManager for setting the flow session on the given app's instance that implements UIApplicationDelegate.

@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

Forgot to mention how I propose on can make use of the lib in a given app:

@interface AppDelegate()<RNAppAuthAuthorizationFlowManager> {
  id <OIDAuthorizationFlowSession> _currentSession;
}
@end

@implementation AppDelegate
-(void)setCurrentAuthorizationFlowSession:(id<OIDAuthorizationFlowSession>)session {
  // retain session for further use
  _currentSession = session;
}
- (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary<NSString *, id> *)options {
  BOOL shouldOpenUrl = [_currentSession resumeAuthorizationFlowWithURL:url];
  _currentSession = nil;
  return shouldOpenUrl;
}
@end

@hakonk hakonk changed the title [fix] add home page in podspec [fix] podspec and making use of UIApplicationDelegate Jun 27, 2018
@kadikraman
Copy link
Contributor

@hakonk that's brilliant, it's been on my To Do list forever, but I've not had the time, really appreciate it, it's definitely the right way to go.

Could you please also update the Example app and the Readme 🙏

@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

@kadikraman Yep! I'm on it now. I do have a question about the Swift section in the documentation. Is it really a use case for React Native apps where AppDelegate is implemented in Swift? While I guess it's doable, it doesn't really provide much convenience, especially since React Native for iOS is heavily based on Objective-C, Macros etc.

@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

Also, I'm having some issues with building the example project. Those issues are not related to this PR however as it seems they have been there for a while. Fixing the example project, I believe should be a separate PR. What are your thoughts?

@kadikraman
Copy link
Contributor

Swift: those instructions were added by a contributor. Figured there was no harm in merging them, although we don't officially support it. If it will no longer be valid after this PR, perhaps it's best just to delete them.

Example app: agreed, definitely a separate PR. It's frustrating, but I can't reproduce the error at all. The example app works for me both from Xcode and terminal on two separate computers.

@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

I did actually manage to make the example project compile 😄 (see latest Podfile and xcode project changes). Regarding Swift, I could just add to the README that it's untested, but should work in a similar fashion?

@hakonk
Copy link
Author

hakonk commented Jun 27, 2018

@kadikraman I've added a suggestion in the README file concerning Swift usage.


```diff
+ @protocol OIDAuthorizationFlowSession;
`RNAppAuth` will call on the given app's delegate via `[UIApplication sharedApplication].delegate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

First step needs to be to add the imports:

+#import <AppAuth/AppAuth.h>
+#import "RNAppAuthAuthorizationFlowManager.h"

@kadikraman
Copy link
Contributor

Just tested your branch on a fresh RN project and it works! 🎉 Just needs a small update to the readme to add the imports to AppDelegate.m.

@hakonk
Copy link
Author

hakonk commented Jun 28, 2018

Glad to hear that! I've added some lines about imports in AppDelegate.

@hakonk
Copy link
Author

hakonk commented Jun 28, 2018

@kadikraman What does your release cycle look like, any scheduled releases in the near future?

@kadikraman
Copy link
Contributor

I want to get this released ASAP, but it will have to be in a v3.0.0 as it's a breaking change.

I was hoping to include another big feature in v3 (separating the token exchange and auth steps) so I'll try and get it finished this week, but regardless, going to release your PR early next week.

@hakonk
Copy link
Author

hakonk commented Jun 28, 2018

Alright, sounds good. Let me know if you need anything from me.

@kadikraman kadikraman merged commit a6f8c0e into FormidableLabs:master Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants