Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Alamofire an optional dependency #34

Closed

Conversation

benasher44
Copy link

@benasher44 benasher44 commented Apr 8, 2018

Issue Link πŸ”—

This fixes #32, making Alamofire optional.

Goals ⚽

  • Allow using AlamofireNetworkActivityIndicator without Alamofire

Implementation Details 🚧

  • This uses the new (in Swift 4.1) #if canImport to conditionally import Alamofire and setup the default notification listening behavior that is currently used by Alamofire users.
  • If Alamofire cannot be imported, no notification listening is setup by default, and clients of this library can now register notifications used by other libraries for this library to listen to.

Testing Details πŸ”

  • Add tests for the public notification method
  • Setup travis to test with and without Alamofire

@benasher44
Copy link
Author

benasher44 commented Apr 8, 2018

As discussed in #32, this will have to wait until we're ready to require Swift 4.1.

For testing "how does this library behave in an Alamofire environment," I was thinking of moving forward with using Carthage to test one environment and CocoaPods to test the other. We can pod lib lint to verify the build without Alamofire, and we can use Carthage and the existing xcodeproj to verify the build with Alamofire. Thoughts? I think the Cartfile will need to be moved into a subdirectory made private, so that Carthage users stop automatically picking up the Alamofire dependency. Is that accurate? I'm less familiar with Carthage. This seems to be the case after reviewing Carthage documentation.

@benasher44
Copy link
Author

I've also updated the version number. I believe this is a backwards incompatible change: users that pulled in this as a dependency and were taking advantage of its Alamofire dependency (as a result) will no longer get that benefit.

import Foundation
import UIKit

#if canImport(Alamofire)
import Alamofire
Copy link
Author

Choose a reason for hiding this comment

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

This approach gathers the separation of Alamofire symbols into one place.

@@ -172,29 +195,29 @@ public class NetworkActivityIndicatorManager {

// MARK: - Private - Notification Registration

private func registerForNotifications() {
public func registerForNotifications(
requestDidStartNotificationNames: [Notification.Name] = defaultRequestDidStartNotificationNames,
Copy link
Author

Choose a reason for hiding this comment

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

This function works particularly well with AFNetworking; you can hook this library up to AFNetworking in one call.

- Cartfile is now Cartfile.private
- Alamofire is now a dependency only for tests
- Updated travis to use xcode9.3 GM
@benasher44
Copy link
Author

Looks like Xcode 9.3 has known problems with running tests with iOS 8.4: http://www.openradar.me/39335367

@benasher44
Copy link
Author

After attempting to write tests for this, it has become clear that accomplishing the goal of making Alamofire optional for a single file adds a lot more complexity than makes sense to try to support. The issue boils down to this: in order to ensure that if #canImport(Alamofire) works properly, you have to be sure that Alamofire is compiled before this library.

The only way to really ensure that is to declare 2 modes of integration for each dependency manager: 1 podspec/Cartfile that is explicit about the Alamofire dependency and 1 that declare no dependency. For Carthage, that may not even be possible to do in one repo; because Carthage picks up the Cartfile at the root of the repo, maintaining a separate fork (forgive my lack of Carthage knowledge if there's something I missed there) may be required.

The actual lines of Swift here seem simple enough, but I think the maintenance burden this adds is too high. I'm going to close this PR and issue. Thanks for entertaining this!

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.

Separate AlamofireNetworkActivityIndicator from Alamofire?
1 participant