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

Updated project to support Carthage #169

Closed
wants to merge 63 commits into from
Closed

Updated project to support Carthage #169

wants to merge 63 commits into from

Conversation

justinmakaila
Copy link
Contributor

This PR adds full support for Carthage, and a solution to using RxSwift and ReactiveCocoa without mandating it for those who wish to use pure Moya.

  • Adds ReactiveMoya and RxMoya targets
  • Adds RAC 3.0 support to ReactiveCocoaMoyaProvider
  • Expands the example project to include demonstrations using reactive extensions

@ashfurrow
Copy link
Member

Thanks a lot, Justin! It's a huge PR – I'll take a look at it this weekend. If any other contributors could lend a hand, that'd be 👍

Justin, could I please ask you to mark these improvements in the changelog? I see you've already updated the README – awesome 🎉

The tests are failing – looks like a Circle configuration problem around the Demo workspace. Any ideas?

@justinmakaila
Copy link
Contributor Author

No idea, I've never worked with circle before. I'll take a look and get back to you about that

—Justin Makaila

On Thu, Jul 16, 2015 at 9:09 PM, Ash Furrow notifications@github.com
wrote:

Thanks a lot, Justin! It's a huge PR – I'll take a look at it this weekend. If any other contributors could lend a hand, that'd be 👍
Justin, could I please ask you to mark these improvements in the changelog? I see you've already updated the README – awesome 🎉

The tests are failing – looks like a Circle configuration problem around the Demo workspace. Any ideas?

Reply to this email directly or view it on GitHub:
#169 (comment)

@colinta
Copy link
Contributor

colinta commented Jul 17, 2015

1,054 files!? That's... ahem a bit alarming.

It looks like all these files are mostly build artifacts. Looking at other projects that support Carthage & CocoaPods, they don't include a Carthage/ folder that contains all the build headers. I'm looking in particular at https://github.com/thoughtbot/argo

I'm wondering: can that entire folder be removed? From what I understand about Carthage, it's a minimalist system, it seems like a red flag that all these header files are included here.

# Conflicts:
#	Demo/DemoTests/MoyaProviderSpec.swift
#	Demo/Pods/RxSwift/RxSwift/RxSwift/Rx.swift
#	Moya/ReactiveCocoa/Moya+ReactiveCocoa.swift
#	Moya/RxSwift/Moya+RxSwift.swift
@justinmakaila
Copy link
Contributor Author

So this is just about ready to go, I just want to get the CI to build successfully.

@ashfurrow
Copy link
Member

Amazing! Let me know if I can lend a hand. Thanks again! 💥

@ashfurrow
Copy link
Member

Hey @justinmakaila – would it be possible to have the Example app and tests still built using CocoaPods?

@justinmakaila
Copy link
Contributor Author

@ashfurrow It'd be possible, but all of that would have to be redone. As it stands:

To run the example:

  1. Run carthage update in the root
  2. Open Moya.xcworkspace and set your target to MoyaExample OR open Example/MoyaExample.xcodeproj
  3. Run.

To execute tests:

  1. Run carthage update in the root (if you haven't already)
  2. Open Moya.xcworkspace or Moya.xcodeproj and set your target to Moya Tests
  3. Run

EDIT: Slight correction, you can't open the MoyaExample.xcodeproj and run the project because it has a target dependency on the Moya.xcodeproj output.

@ashfurrow
Copy link
Member

So Carthage relies on the Moya xcodeproject? I thought CocoaPods was generating one for us now. Sorry if I'm showing my ignorance here – I'm sure you can understand my hesitation to make Moya depend on an unfamiliar tool.

@justinmakaila
Copy link
Contributor Author

Yes. CocoaPods was symlinking to the Pods project, but I found it to be overkill. I totally understand your hesitation, and want to do everything I can to make the transition simple, and answer any questions you have along the way.

@ashfurrow
Copy link
Member

OK, so. Touchy subject for some – I want anyone reading this to know that I have a lot of respect for the developers who've built Carthage, for @justinmakaila and @neonichu who have done so much work to support Carthage in Moya, and for developers using Moya. If someone tries to turn this into a flamewar, I'll be very disappointed in them.

❤️

Ok! So Moya actually predates Carthage by a few months – Moya was build as a CocoaPod, which the capabilities and limitations of that tool in mind.

Carthage was released, and people asked for Carthage support on Moya. I didn't know how to add it, but several contributors offered to help, which is fantastic 😸 They asked for help from Carthage, but their developers said that subspec-like functionality conflicts with their philosophy.

Undeterred, @justinmakaila and others laboured to come up with a solution that worked for both tools, requiring Moya to compromise on the original library structure (informed by CocoaPods). At last, we'd nearly accomplished this herculean task.

My remaining issue is that this PR fundamentally changes Moya from a CocoaPod that could support Carthage, to a Carthage library that can incidentally be used with CocoaPods. This might seem like a trivial matter, and it's at this point that I need to recognize that I no longer own Moya – the community does. My own feelings matter a lot less.

But the community has been using Moya as a CocoaPod for its entire lifetime, and a change of this foundational nature needs to be considered carefully.

Respectfully, Justin, I think the Demo, unit tests, CI, and all that stuff should remain built atop CocoaPods. I realize this undoes some of the work you've put into this, so I'd like to do the work: I'll check out your fork, get Moya working atop CocoaPods again, ensure that Carthage support still works, then send a PR back into your branch so you can review my changes. Does that sound OK to you? We can hop on a google hangout to discuss this in person, too!

@colinta
Copy link
Contributor

colinta commented Aug 21, 2015

I'm glad the focus is remaining on CocoaPods. I think Carthage is a great tool, but since most users aren't clamoring for supporting it, I think we'd be doing a disservice if CocoaPods was made a second class citizen.

@ashfurrow
Copy link
Member

Hey! This is still on my radar, just got sidetracked.

As for the code structure, having the separate repos like https://github.com/Moya/ReactiveMoya is fine for code, but I think it's a good idea to keep all documentation and issue tracking in this main repo. This will keep the barrier to entry low (someone learning to use Moya shouldn't have to know how we structure our code to read our docs). Any objections?

@justinmakaila
Copy link
Contributor Author

None here

—Justin Makaila

On Thu, Sep 10, 2015 at 1:31 PM, Ash Furrow notifications@github.com
wrote:

Hey! This is still on my radar, just got sidetracked.

As for the code structure, having the separate repos like https://github.com/Moya/ReactiveMoya is fine for code, but I think it's a good idea to keep all documentation and issue tracking in this main repo. This will keep the barrier to entry low (someone learning to use Moya shouldn't have to know how we structure our code to read our docs). Any objections?

Reply to this email directly or view it on GitHub:
#169 (comment)

@buildreactive
Copy link

I agree – as someone that definitely falls into the "new user" category. I'm just now trying to get Moya, ReactiveCocoa, and Alamofire working together... and the fewer things I have to hunt down and figure out, the better. I also vote for keeping a focus on CocoaPods. I have one Carthage module (because they don't have a pod yet... but should soon). Pods work much better for us, in a distributed project.

@ashfurrow
Copy link
Member

Alrighty, I've disabled issues on the other repositories. We also need to update their READMEs to point to this repository. These other repos are basically just places to store files.

If we're centralizing docs here, I think it makes sense to centralize tests here as well. I can't see that being a problem with Carthage – @justinmakaila does that sound OK?

@justinmakaila
Copy link
Contributor Author

As long as the reactive repositories can be imported into the test suite without naming collisions (They both have their product names set to “Moya”). 

—Justin Makaila

On Thu, Sep 10, 2015 at 1:44 PM, Ash Furrow notifications@github.com
wrote:

Alrighty, I've disabled issues on the other repositories. We also need to update their READMEs to point to this repository. These other repos are basically just places to store files.

If we're centralizing docs here, I think it makes sense to centralize tests here as well. I can't see that being a problem with Carthage – @justinmakaila does that sound OK?

Reply to this email directly or view it on GitHub:
#169 (comment)

@ashfurrow
Copy link
Member

Shouldn't be a problem – we're restructuring the tests to run under CocoaPods, which won't have that problem.

@justinmakaila
Copy link
Contributor Author

👍🏻

—Justin Makaila

On Thu, Sep 10, 2015 at 2:04 PM, Ash Furrow notifications@github.com
wrote:

Shouldn't be a problem – we're restructuring the tests to run under CocoaPods, which won't have that problem.

Reply to this email directly or view it on GitHub:
#169 (comment)

@ashfurrow
Copy link
Member

Orta and I have implemented a solution that doesn't require us to split up the source code into three separate repositories, which I've PR'd in #215. @justinmakaila thank you so much for your work on 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.

None yet

5 participants