search

PeterStaev / nativescript-azure-mobile-apps

:cloud: NativeScript plugin for working with Microsoft Azure Mobile Apps services
Apache License 2.0
30 stars 10 forks source link

Switched to SafariViewController to comply with the In-App Browser ban #14

Closed EddyVerbruggen closed 7 years ago

EddyVerbruggen commented 7 years ago

Work in progress (do not merge yet), I need to document the changes

Hi,

Sending this already for you to review. I will document the required changes in the readme. But that's really all there's left.

Eddy

PeterStaev commented 7 years ago

Hey @EddyVerbruggen , it looks great!. Have one question though - is it really needed we require the user to manually set the delegate? How about we do it internally in the if where you reject the login here? Won't this be a better option and should still work?

EddyVerbruggen commented 7 years ago

Hey Peter,

I wondered the same and tried and tried and tried but it seems you must really wire the appdelegate before the app is started. I had a similar experience with my Firebase and 3D Touch plugins.

It could be simplified to only a require and do the delegate assignment in the required plugin file, but I'm afraid we need the user to add that line.

There is an alternative but I'm not intending to do that now: add a native library that swizzles the required appdelegate methods and ship it with the plugin.

PeterStaev commented 7 years ago

True. Seems related to this: https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/application/application.ios.ts#L285 I guess Apple did not account for users to be able to change the AppDelegate during runtime and they require you to specify it when you start the app 😄

PeterStaev commented 7 years ago

Thinking more about this, how about we add static function in the root module of the plugin that will do the delegate registration? We can have a dummy android file with the function doing nothing. This way the user will not have to do a check for platform and will just have to call the function before application.start(). Thoughts?

EddyVerbruggen commented 7 years ago

Hey Peter, indeed. That's kinda what I meant by "It could be simplified to only a require and do the delegate assignment in the required plugin file, but I'm afraid we need the user to add that line." but you've thought it a bit more through.

Let me take a stab at that tonight or this weekend.

EddyVerbruggen commented 7 years ago

iOS AppDelegate wiring has been refactored. Still need to do the doc 😄

EddyVerbruggen commented 7 years ago

Docs are done. It should be good to merge. I've seen travis failing on previous commits and it seemed unrelated to this PR really, but not sure. Can you look at those please?

PeterStaev commented 7 years ago

Thanks @EddyVerbruggen ! I will merge a bit later and will see to fix Travis (I think i know what is the problem)