[discussion] Expose ability to login with custom method + examine response object

[alexcorre]

Expose loginWithMethodName:parameters:completionHandler: in METDDPClient.h. Use METMethodCompletionHandler instead of METLogInCompletionHandler to expose the login response body.

Not sure if this is the right way to go about it but figured i'd send a PR to start a discussion on how to achieve the following:

It is helpful to us to be able to see the login response object as it contains some valuable tracking information (ie whether a new account was created.) in addition to the login tokens/userId.

We have a custom login method that accepts a facebook auth token directly and returns the users meteor credentials. Details on this type of custom login can be seen in our response to the following stack overflow issue here

This PR is what I'm working with now to achieve this. Any feedback on how to expose this power more cleanly from meteor-ios is welcomed.

[martijnwalraven]

I'm wondering if there isn't a better way to achieve what you need, as Meteor itself doesn't expose the result object either it seems, not even in the internal login function (see here).

That being said, changing loginWithMethodName:parameters:completionHandler: to take a METMethodCompletionHandler seems like the best solution for exposing the result. It does complicate writing public login methods (such as loginWithEmail:password:completionHandler) because you need to translate between METMethodCompletionHandler and METLogInCompletionHandler. And I'd prefer to find a way to not expose it as part of the public API if possible. Is there any way your code could depend on METDDPClient_Internal.h?

[alexcorre]

To your first point, I do agree there is probably a better way to achieve what I've described since it is indeed not exposed in meteors internal login function.

So exposing this in the public header probably isn't the right thing to do. I can keep the function in METDDPClient_Internal.h but will have to expose that file to applications consuming the Pod as a public header in Meteor.podspec. Would that be ok?

As far as translating between METMethodCompletionHandler and METLoginCompletionHandler its a small price to pay for giving users the ability to create custom login methods.

I'll mess around a little more with this and see if I can do something cleaner that you like.

If we dont want to expose _internal.h or change the existing API we can add a separate category...perhaps METDDPClient+CustomLogin that exposes a new customLogin method that takes an METMethodCompletionHandler instead of the login completion. Will require a bit of refactoring to share the current implementation internally.

[martijnwalraven]

Ideally, I'd like to make it possible for new account methods to be distributed separately (e.g. similar to the accounts-password, accounts-facebook, etc. packages in Meteor). But I don't know how to accomplish this cleanly. I'm not familiar enough with CocoaPods subspecs and would prefer to be independent of any particular dependency mechanism (especially now that we have dynamic frameworks and alternatives like Carthage).

For now, I think the pragmatic solution would to be to change loginWithMethodName:parameters:completionHandler: to expose the result as you have done, and keep writing login mechanisms as separate categories (AccountsPassword, AccountsFacebook, etc.). Depending on how custom your login mechanism is you could either integrate it (it would be great to have a more general Facebook login mechanism that worked with iOS accounts available) or add the category as part of your own project.

I'd prefer not to expose METDDPClient_Internal.h as a public header, so adding a separate header (METDDPClient_Accounts.h to be consistent with Meteor accounts-base?) would be cleaner. You could then add it as a public header to the Podspec (unless you know of a way to cleanly separate the public API from an accounts SPI using subspecs or some other mechanism, but that might be overdoing it for something as simple as this).

[alexcorre]

Agree that account methods should be in categories and should be able to be distributed separately. I'm in the same boat as you on the CocoaPods knowledge, but I'm sure its not that complicated after a bit of reading up on it.

As far as our generic facebook login code, it works well for us, but since method is a custom one, it is not officially supported by meteor so dont think it belongs in this library right now. We can look into submitting a PR for it to meteor, but this will take some time to happen.

There are two issues here. One is how to expose the loginWithMethodName:parameters:completionHandler: to the consuming application. The other is how to expose the login response body to the consuming application, only if desired, ideally without changing current apis.

First issue: I think it will be fine to expose this method in a separate public header with this method as you have said (ie METDDPClient_Accounts.h). Doing this and exposing it as a public header will allow consuming applications to develop custom login methods that are not included in meteor-ios package, which i think is a good idea. If we dont expose this method somehow in a public header consuming applications will be forced to do an in app class extension to gain visibility into the method, this is all well and good in Objective-C but I dont think this would be available in a pure Swift app.

Second issue: Changing the API as I have done is an ok solution. I just thought of another that may be cleaner. We leave the existing loginWithMethodName:parameters:completionHandler as is with the METLoginCompletionHandlertype of completionHandler. This implementation will call a new method called loginWithMethodName:parameters:responseHandler:completionHandler: which actually does the work and calls the responseHandler with the login response, if its successful, and a responseHandler is included.

I'll try to get my new ideas/cleanup into this PR so we can see it in action sometime tomorrow.

[alexcorre]

Tried to implement what I just suggested in the last commit. Not sure if this way is cleaner or not than the first. Let me know what you think.

There are a lot of diffs since I had to change the podspec to expose METDDPClient_Accounts.h and rerun pod install with the latest rc of cocoapods for the demos to work. Also since the Pods/ directory is not in the .gitignore you see all the changes there.

Hopefully you can sift through it to see the good stuff.

[martijnwalraven]

Thanks! I'm not sure if having a variant with two completionHandlers is cleaner. I think I prefer one consistent method. Changing loginWithMethodName:parameters:completionHandler: to take a METMethodCompletionHandler as you did in your original commit is probably the simplest solution for now. I like having a separate METDDPClient_Accounts.h for the SPI though.

I agree checking in the Pods/ directory is a pain. It is what the CocoaPods people recommend, but it makes it really hard to keep commits clean if anything in the Podspec changes. We could add it to the .gitignore, but that would force everyone to use CocoaPods because you wouldn't be able to build the framework without running pod install first to download the dependencies.

Because I'm working mostly with iOS 8 and Swift now, I've been looking at other approaches like Carthage, which can be used with Git submodules. But this still seem immature and CocoaPods is much more widely known. I'm not sure what to do here. Do you have any recommendations?

[justinmakaila]

I think this issue would be best solved by building individual submodules within the Meteor module. It would allow something like import Meteor.AccountsPassword or import Meteor.AccountsFacebook

Obviously, import Meteor would also import all of the submodules, which is not exactly how it's modeled in the js, but it could allow a separation of concerns by allowing the end-user to import each specific module.

[martijnwalraven]

Using submodules seems ideal, but I'm not sure how this would work. Do you know anything about splitting framework targets into submodules? You would probably have to specify your own module map. But that wouldn't give you separate binaries I think.

Maybe using separate frameworks would make more sense? Using MeteorAccountsFacebook.framework is not as pretty, but might be more practical. It could also have its own dependencies (e.g. FacebookSDK).

With CocoaPods you'd probably use subspecs here, but these would all be built into a single library/framework.

[justinmakaila]

The nice thing about Swift and dynamic frameworks is that every class within the framework can be imported individually.

For instance I could create a new class and import the entire ReactiveCocoa module (import ReactiveCocoa), but maybe when I'm finished writing the class, I realize that I only use the RACSignal class. I can change my import to import ReactiveCocoa.RACSignal

Separate frameworks appears to be the best solution, as you can create a framework called MeteorAccounts which depends on FacebookSDK, TwitterKit, and Meteor. The down side is that this would then require the end user to think "my dependencies are Meteor, and MeteorAccounts if I want to log in!" which I'm not sure is desirable.