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

Move Realm integration to another repository #1756

Closed
akashivskyy opened this issue Oct 31, 2016 · 20 comments
Closed

Move Realm integration to another repository #1756

akashivskyy opened this issue Oct 31, 2016 · 20 comments

Comments

@akashivskyy
Copy link

Have you considered moving ChartsRealm to another repository, that depends on both Charts and Realm?

Right now, they are both in this repository and in the same project. That causes some dependency managers, like Carthage, to build all Charts, ChartsRealm and Realm itself, even if its integration is not used.

@danielgindi
Copy link
Collaborator

We have. A lot. Unfortunately this complicates things...
ChartsRealm needs Charts and Realm. ChartsDemo is obviously managed as part of the Charts repository for simplicity and accessibility.
As ChartsDemo provides Realm demos as well, it depends on Charts and ChartsRealm.

So this makes: Charts repo will depend on ChartsRealm repo which will depend on Charts repo.

Currently, both Pods and Carthage do not support any kind of cyclic dependencies. They did not implement even the simplest algorithm to find redundancies like this and resolve them, as in practice it can get complicated.

So for ease of use we are keeping things as it is right now.
If there will be more votes for having this separated along with its demos, we may consider this.

I'll leave this open for now.

@akashivskyy
Copy link
Author

akashivskyy commented Oct 31, 2016

I understand, thanks for the quick answer!

In the meantime, I think I'll create a quick fork and remove Realm dependencies in there. I hope this will resolve my 20-min-long carthage update 🙈

@danielgindi
Copy link
Collaborator

Make sure to update your fork frequently, as important bugfixes and new features come in....

@pmairoldi
Copy link
Collaborator

pmairoldi commented Oct 31, 2016

If you find Carthage takes too long use cocoapods. You can specify to only build charts and not chartsrealm since they have subspecs.

I tried out Carthage a while back but went back to cocoapods because it's faster and simpler.

@akashivskyy
Copy link
Author

akashivskyy commented Oct 31, 2016

That's subjective. For me, CocoaPods is easier, but Carthage is simpler. 😉

@pmairoldi
Copy link
Collaborator

Ya I meant easier ;)

@calvingit
Copy link

@danielgindi can you update realm to 2.x ?

@pmairoldi
Copy link
Collaborator

Pull requests are always welcome 😉

@getaaron
Copy link

@danielgindi Seems like the current ChartsDemo is really a ChartsRealmDemo. You could move this demo and Realm to a separate project, and provide a bare-bones ChartsDemo with no realm dependencies. Just add a note that a more full-featured demo is available in the ChartsRealm project.

Moving ChartsRealm out of Charts will save our dev team lots of pointless download and building time.

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 30, 2016

@getaaron, not all of it, ChartsDemo still holds more demos without realm. I agree we could separate them, but so far, I just needed one time building and downloading for Realm, by carthage. It should not build all the time?

@Antoine4011
Copy link

@danielgindi I'm in favor to move ChartsRealm to another repo.
This take so long to build with carthage...

*** Building scheme "RealmSwift" in Realm.xcworkspace
*** Building scheme "Realm" in Realm.xcworkspace
*** Building scheme "PlaygroundFrameworkWrapper" in RealmExamples.xcworkspace
*** Building scheme "ChartsRealm" in Charts.xcodeproj
*** Building scheme "Charts" in Charts.xcodeproj

This make me crazy because I only use Charts...

@pmairoldi
Copy link
Collaborator

Well Carthage really doesn't want to add subspecs so if you don't want to download everything all the time use cocoapods or complain to the Carthage maintainers. Separate repos make our lives much harder for little gain. We could split the demos into 2 targets but that really doesn't fix some of the problems mentioned here.

@donnellyk
Copy link

donnellyk commented Dec 15, 2016

+1 to removing the Realm dependency.

It isn't a core part of the Charts library or even related; it is a nice to have. You already did the work to split the dependencies, so separate repo seems to make sense. AlamoFire does the same thing.

For me, it isn't about build times or anything, just project maturity. Seeing Realm pop up unexpectedly in Cartfile.resolved threw a major red flag in my team.

@getaaron
Copy link

I agree with @donnellyk.

@danielgindi
Copy link
Collaborator

@donnellyk Until we think of a solution that answers the major concerns - we can't do that. And believe me, we want to!

@akashivskyy
Copy link
Author

@danielgindi Fair enough. Which concerns are that? Maybe we can help? 🙂

@danielgindi
Copy link
Collaborator

At first we thought about keeping the Realm stuff in the project - for ease of maintainability.
The problem arising from it is cyclic dependencies not being supported by both CocoaPods and Carthage.

Now we're left with the desire to keep the Demo with the showcase of Realm related stuff, both for the users to be accessible, and for us when testing while developing.
Think about this: There's a bug in Charts which affects ChartsRealm, or a utility function that we're adding in favor of ChartsRealm. The development process would be to either:

  1. Do stuff on Charts clone, copy over to the Pods folder in ChartsRealm clone, see that it Builds, then run the Demo and test it visually, then maybe run some unit tests.
  2. Do the same on the Pods folder in ChartsRealm, and then copy over to Charts clone.

The original main concern was more about the users: There's very high demand to ChartsRealm. Current situation makes ChartsRealm very visible and accessible.
But now that we get many rejects from vanilla Charts users, who are not aware that Realm is just an optional dependency (or that it needs to be fetched for the Demo to work) - I'm thinking our concerns were upside-down.

About developing flow - Maybe we should just bite the bullet, get used to it. There's not much develpoment going on on the ChartsRealm side anyway (except for implementing the new ObjC support stuff on Realm which was released a few days ago following my PR).

I was generally very busy lately with many things - so I want to sit down and close some issues, merge PRs etc., and then maybe try to separate the ChartsRealm repo and see how it goes.

@liuxuan30
Copy link
Member

@danielgindi finally catch you here. Yes we have many things to fix before thinking about Realm.. 😂

@danielgindi
Copy link
Collaborator

Yeah it's been some busy days :-) Still are...
But I'm taking care of some issues and PRs, moving towards the next minor release,
and at that time we'll probably separate realm stuff.

@danielgindi
Copy link
Collaborator

Okay so I'm done splitting the repo and moving the realm stuff to https://github.com/danielgindi/ChartsRealm.
Travis is still broken, @petester42 could you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants