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

Changed linkage of RxSwift/RxAtomic to optional #74

Closed
wants to merge 1 commit into from
Closed

Changed linkage of RxSwift/RxAtomic to optional #74

wants to merge 1 commit into from

Conversation

scogeo
Copy link

@scogeo scogeo commented Mar 19, 2019

Please consider making the linkage to RxSwift/RxAtomic optional.

XCoordinator currently requires RxSwift/RxAtomic frameworks to to be linked to the final application when built using Carthage. Changing the linkage of RxSwift/RxAtomic to optional in the XCoordinator framework removes the dependency.

@pauljohanneskraft
Copy link
Collaborator

Thanks for the contribution to XCoordinator!

Since we already made RxSwift and RxAtomic optional from a code perspective, this definitely looks to be a good addition for Carthage users.

@pauljohanneskraft
Copy link
Collaborator

I have just tried this with an example project and using import RxSwift does not cause any problems (and XCode offers me code completion for it) even though I did not specify it in the Cartfile.

My Cartfile looks as following, the project is the one-page template:

github "https://github.com/scogeo/XCoordinator" "rx_optinal_linkage"

Am I doing something wrong or can we maybe get optional linkage to RxSwift/RxAtomic using another way?

@pauljohanneskraft pauljohanneskraft self-assigned this Mar 19, 2019
@scogeo
Copy link
Author

scogeo commented Mar 20, 2019

First, thanks for making such a great coordinator framework. I've only been playing with it for a couple weeks, but really like it.

I'm surprised you were able to import RxSwift in the sample project without putting in the Cartfile. I was going to try and reproduce this, but I hit a different issue when I submitted this optionally linked version as part of a project to Test Flight on the App Store. It failed Apple's validation checks because they don't allow you to have libraries with optional (but missing) dependencies in a bundle on the AppStore even though it runs fine on the simulator and my devices. I'm sure it is for security reasons.

So you can disregard this push request. For now I'm just going to add XCoordinator sources directly to my project as a submodule.

One possible solution for Carthage users would be to add two separate targets that Carthage would build one with and one without the RxSwift additions, just like you have done with Pods. Then the user would just pick one of them to link with their project. Your Xcode project structure is a little unorthodox for a Carthage build, so it would require adding a new universal/empty .xcodeproject file and creating two framework targets and shared schemes for carthage to use (and then removing the shared scheme for the XCoordinator framework in the Examples directory that Carthage is currently building)

Or if you use the existing project structure you may be able to make another "example" target in your example directory and remove the "XCooodinator/RxSwift" pod for that target, which should result in another copy of the framework without RxSwift to be available. But that is a bit of a hack.

@pauljohanneskraft
Copy link
Collaborator

Great to hear you like it!

I'm closing this PR since it did not solve the issue so far and created #76 for discussion and as a reminder. The project structure is somewhat given as XCoordinator uses the Cocoapod template for creating pods. I'd prefer not to change the project structure and rather use multiple shared schemes, if possible.
I will have to check for possible solutions and try what works and what doesn't and will report back in the referenced issue.

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.

None yet

2 participants