search

nstack-io / nstack-ios-sdk

MIT License
5 stars 10 forks source link

SPM Support (WIP) #65

Closed jakobmygind closed 4 years ago

jakobmygind commented 4 years ago

Hi guys, first stab at SPM Support as I really need it now. ~I get the dreaded crash when putting the translations in another module, but seems to work otherwise~. ~I took some shortcuts as I only need iOS, so some fixing is probably needed for other platforms.~ I think it might work for other platforms as well, but some of the code was a little coupled with UIKit. This is not ready for merge nor is it certain it will ever be, but it might be a starting point for someone if SPM-support is on the roadmap.

If someone takes it for a spin pls let me know how it went

BR

Jakob

jakobmygind commented 4 years ago

@chriscombs Hope you are well! This might be of interest to you.

jakobmygind commented 4 years ago

@chriscombs Just found out, the crash I got was not caused by using Translations from other module, but by using the generated SKLocalizations.swift instead of Localizations.swift. I made a ticket here: https://github.com/nstack-io/nstack-ios-sdk/issues/66#issue-707228398

jakobmygind commented 4 years ago

@mariusc Do you also know if SPM-support is in the works? This PR works pretty well, at least for iPhone for sure, but I feel a little uneasy pointing all our production code towards a branch in my own repo for an SDK I don't maintain 😅

pbodsk commented 4 years ago

Hey @jakobmygindUFST, long time no see 😄

I tried giving it a shot this morning but the app I'm currently working on requires support for iOS 12 and "your" Package.swift defines minimum target to be 13.0.

I tried upping my minimumversion 13.0 and it seems to work OK. The app launches, there are translations...what more do you want really? 😉

So...the initial smoke test was positive, lets see if we can get this merged into our codebase somehow.

jakobmygind commented 4 years ago

Hi Peter! Good to hear from you! Yes, the target, I can try and lower it to 12, I don’t see any problems with that off the bat. I think the localization manager is the limiting factor. As mentioned I had to do some stuff involving conditional compilation and moving of files, so that should probably be reviewed if this PR should be considered for merging. I see some checks are failing on Bitrise but I cannot see which, but it is probably the xctests which are not all passing yet.

Sent from my iPhone

On 14 Oct 2020, at 07.47, Peter Bødskov notifications@github.com wrote:

 Hey @jakobmygindUFST, long time no see 😄

I tried giving it a shot this morning but the app I'm currently working on requires support for iOS 12 and "your" Package.swift defines minimum target to be 13.0.

I tried upping my minimumversion 13.0 and it seems to work OK. The app launches, there are translations...what more do you want really? 😉

So...the initial smoke test was positive, lets see if we can get this merged into our codebase somehow.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

pbodsk commented 4 years ago

I'm sanity checking with the rest of the gang now but I'm thinking we could branch develop out to a feature/spm-support branch and then you could point your PR to that. That way we don't initially trash develop and the "responsibility" moves to us

jakobmygind commented 4 years ago

Sounds like a good solution, unless you already have some working code of your own. I noticed a branch named like that but it was stale and from March so I assumed it didn't pan out. That was before assets were supported so that might be why.

AFAIK edits from repo owner is turned on, so anyone from Nodes can update the PR if needed.