[WEB-749] ReactiveExtensions and ReactiveSwift to SPM #1745
Conversation
β¦ough SPM, included ReactiveSwift as a dependency. Almost build except for a few import ReactiveExtensions (could not find) build errors.
msadoon
changed the title
msadoon
changed the title
|
@msadoon I went ahead and addressed the outstanding issues for this as I was testing on my M1.
|
| 1998BCAD28F60EB700D04077 /* ReactiveExtensions */, | ||
| 1905787E28F8CD5E00428375 /* ReactiveSwift */, | ||
| 6047E6F928FDC104003A55A0 /* ReactiveExtensions */, | ||
| 6047E6FB28FDC1B1003A55A0 /* ReactiveSwift */, |
There was a problem hiding this comment.
Not sure why these IDs changed because the same libraries are linked. It should be ok.
There was a problem hiding this comment.
Usually ID's change when libraries are added and removed, wouldn't worry too much about this as they generated by XCode, not us.
Codecov Report
@@ Coverage Diff @@
## main #1745 +/- ##
=======================================
Coverage 85.38% 85.38%
=======================================
Files 1275 1275
Lines 115181 115181
Branches 30453 30454 +1
=======================================
+ Hits 98350 98352 +2
+ Misses 15778 15776 -2
Partials 1053 1053
π£ Weβre building smart automated test selection to slash your CI/CD build times. Learn more |
msadoon
force-pushed
the
feature/reactive-swift-extensions-spm
branch
from
b4cffd4
to
16910fe
Compare
β¦rk-iOS Because KsApi includes ReactiveSwift and ReactiveExtensions, and Library includes KsApi and KickstarterFramework-iOS includes Library-iOS, there is no need to include RE and RS in either Library-iOS or KickstarterFramework-iOS.
β¦equired for on device build.
Hey @scottkicks, appreciate your investigation here, and I did do some digging on my own before your commit was pushed and found that the changes in your commit were not the root of the issues.
Also worth noting that All of that to say I was curious as to how the build system worked, and while not understanding most of the lower level details, watching this WWDC video (12:54) on the iOS build system gave me the impression that keeping our build phases as minimized as possible gives us faster build times by parallelizing our build dependencies as much as possible. So saying "include the library" might cause unneeded lookups for the build system everytime if its not needed. |
|
@msadoon Ah yea ok. I was going to check out the WWDC video as well. I was finding it strange that CI tests would pass with those extra libraries being linked, but locally they would fail for me. I don't fully understand the lower level details either, but your explanation makes sense π Thanks for writing this extra info out. Running locally now to verify the changes and will approve once done. |
π² What
Our final two Carthage dependency Reactive-Extensions and ReactiveSwift needs to move to SPM.
π€ Why
Part of a platform upgrade to move all our dependencies away from Carthage and shell scripts building for non-M1 architectures to more dependable Swift Package Manager. Context.
π How
This requires us making our internal ReactiveExtensions library SPM compatible. Here is the diff on the changes, but essentially what that requires is:
Package.swiftfile.Sourcesdirectory. Put tests inside aTestsdirectory.import Foundationin 3 files (KsrDebounce.swift,KsrDelay.swift,WithLatestFrom.swift) not sure why exactly, but seeing as this is the most basic of dependencies provided by Apple and the foundation for almost every other framework/library in existence on iOS, it is safe.swift package dump-packageto validatePackage.swiftThe above were changes to the
ReactiveExtensionsrepo to make it SPM compatible. After importing that library via SPM into our Kickstarter project, we includeReactiveExtensionsinto our main app targetKickstarter-iOS,Library-iOS,Kickstarter-Framework-iOSandKsApi. We also includeReactiveExtensions-TestHelpersinto the test targets of the aforementioned libraries.As for
ReactiveSwiftwe tried to use the included dependency withinReactiveExtensionsinstead of importing it via SPM separately but ended up with this strange linking error not being able to link it it through the RE package.Without spending too much time on it, already been about a day, safest is to import it via SPM and include it in all targets with
ReactiveExtensions. This affects the app size and we've already increased it by about 25% by moving the existing Carthage dependencies over. It will increase more with these two big dependencies now moved as well. This means longer install times and longer app start times. But with our newer initiatives to remove existing unused features in the app, we can start to look at things like optimizing app size and build times. Especially for noticeably long start times from first install and subsequent launches.The strong argument for this change (and the reason we did all this platform work) is that developer efficiency is increased substantially due to all builds now working on M1 processors. It's actually twice as fast as a non-M1 MBP after a clean + build.
2019 Intel i9 8-Core 32GB
2021 M1 Pro 16GB
The size/launch time increase seem to be worth it, but we'll do more testing and follow up.
β Acceptance criteria
β° TODO
import ReactiveExtensionsinKsApiService+RequestHelpersfile...