search

gurisko / cordova-plugin-accountkit

AccountKit Plugin for Apache Cordova which allows you to add passwordless email and SMS authentication to your app
MIT License
34 stars 42 forks source link

Allow user to choose between using access tokens and codes #13

Closed Nicolaidavies closed 7 years ago

Nicolaidavies commented 7 years ago

My attempt at solving #9

Definitely still needs some work and error handling. Any help would be much appreciated.

Might want to set options default from JS side, currently there are no defaults and will crash if the options object is not passed with a useClientAccessToken value.

I've also not touched the iOS side.

gurisko commented 7 years ago

Hey @Nicolaidavies! Thank you very much for this PR. It looks great! Do you have any plans on looking at iOS part?

Nicolaidavies commented 7 years ago

I'm looking into the iOS part now, although I've never developed a cordova plugin before.

My issue right now is that the plugin is initialised in pluginInitialize in AKPlugin.m, where the response type is passed in. I haven't found a way to access any passed arguments from the JS side in this method, and I don't think this is the way forward.

The structure of the code seems very different from it's android counterpart. An initial thought is to initialise AccountKit in the loginWithEmail and loginWithPhoneNumber methods, or a create a method that initialises and then calls either of the two methods (like in the android version). This does however seems to go against the cordova ios plugin guidelines.

Maybe it should be approached from a different angle, by first having a AccountKitPlugin.init(options) from the JS side and then allowing the user to call the appropriate login method. I'm personally not a fan of this approach, but it shouldn't be excluded.

Whenever the first part is fixed, the didCompleteLoginWithAuthorizationCodein AKPluginViewController.mwill also have to be implemented. But that seems like a minor thing, compared to the first part.

So I'm a bit stuck. Mimicking the structure of the android counterpart however, seems like the way forward to me.

Let me know what your thoughts are.

gurisko commented 7 years ago

An initial thought is to initialise AccountKit in the loginWithEmail and loginWithPhoneNumber methods

This seems like a way to go imho.

This does however seems to go against the cordova ios plugin guidelines.

Can you please direct me which part are you referring to?

gurisko commented 7 years ago

Any progress on this @Nicolaidavies ?

Nicolaidavies commented 7 years ago

@gurisko apologies for not getting back to you. I haven't spent time looking at it. Hopefully I can make some time soon.

With regards to the iOS guidelines part, the documentation says the following, under "Plugin Initialization and Lifetime":

Plugins should use the pluginInitialize method for their startup logic.

1N50MN14 commented 7 years ago

@gurisko @Nicolaidavies Maybe I'm missing on something here:

  1. The plugin does use pluginInitialize so we're good there https://github.com/gurisko/cordova-plugin-accountkit/blob/master/src/ios/AKPlugin.m#L10

  2. Why not just move the if statement / init https://github.com/gurisko/cordova-plugin-accountkit/blob/master/src/ios/AKPlugin.m#L11 to loginWithPhoneNumber and loginWithEmail and add a check for useAccessToken`?

My Swift skills (or whatever language that is) is comparable to valkarian but it should be pretty straight forward I guess..?

gurisko commented 7 years ago

@Nicolaidavies I will take over this since it seems to not be updated for almost 2 weeks already. Feel free to follow the process in login-flow branch.

Nicolaidavies commented 7 years ago

@gurisko Thank you and apologies for not getting back. I currently don't have access to an iPhone, so that makes it hard to work on.