robbiehanson / XMPPFramework

An XMPP Framework in Objective-C for Mac and iOS
Other
5.91k stars 2.09k forks source link

[carthage] handling XEP extensions #881

Open dodikk opened 7 years ago

dodikk commented 7 years ago

The carthage support pull request https://github.com/robbiehanson/XMPPFramework/pull/870 creates a framework for core only.

There should be some policy to handle the extensions (various XEPs). Possible solutions :

  1. One large framework with both core and all extensions
  2. A framework per extension

Dear maintainers, please share your ideas.

P.S. During the implementation, don't forget to fix CoreData related code should be fixed as the models would not be stored in [NSBundle mainBundle] any longer.

chrisballinger commented 7 years ago

I modified the PR to bundle all extensions, to match what we're now doing with the podspec. The amount of complexity involved in conditionally including various extensions was too high and a burden on maintenance. The amount of code bloat is minimal.

The main problem is that XMPPFramework.h was designed to be user configurable, but that's not really an option without adding major headaches. I think that the old #ifdefs feature checks could be replaced by runtime checks, or removed entirely. For example, the checks for _XMPP_CAPABILITIES_H:

#ifdef _XMPP_CAPABILITIES_H
    [xmppStream removeAutoDelegate:self delegateQueue:moduleQueue fromModulesOfClass:[XMPPCapabilities class]];
#endif

https://github.com/robbiehanson/XMPPFramework/blob/master/Extensions/XEP-0045/XMPPMUC.m#L67

dodikk commented 7 years ago

I modified the PR to bundle all extensions

Good to hear you've taken care of the extensions. How about my concern related to CoreData model files?

dodikk commented 7 years ago

the old #ifdefs feature checks

I guess, I've missed that part. Do you have any reference describing all such flags?

chrisballinger commented 7 years ago

@dodikk Good call, both the podspec and framework should use a resource bundle because that's considered best practice. Tracking here: https://github.com/robbiehanson/XMPPFramework/issues/883

The ifdef feature checks are scattered throughout the codebase and depend on the XMPPFramework.h file including other files. There should generally be minimal side effects to including everything, because it still won't do much unless you also have activated the module on your stream. One example is that XMPPRoster will process incoming groups differently if the XMPPMUC module ifdef is present, but this is the only way to result in the correct behavior. If the XMPPMUC ifdef isn't present you will end up with garbage roster items, so I think this behavior is overall better, even if you haven't activated the XMPPMUC module.

dodikk commented 7 years ago

@chrisballinger , I guess a separate resource bundle is not needed for the framework since the framework is already a bundle. Still, it might make sense for the sake of a consistent approach supporting both cocoapods and carthage.

P.S. I am using the term "bundle" for NSBundle