Swift concurrency

[rcasula]

Update the Library to the new swift-concurrency standard.
I was thinking if removing the composable architecture dependency makes sense.

This PR is still in draft because I have to:

• update documentation
• eventually remove the composable-architecture dependency

Please let me know if this PR is any good!

[stephencelis]

@rcasula Thanks for looking into this! We definitely have plans to adopt this new style soon, but haven't had time to look into it. This looks good! Just a couple things we need to figure out in the future:

1. Does the manager need to be protected in an actor? And what concurrency warnings show up when we turn on -Xfrontend -warn-concurrency -Xfrontend -enable-actor-data-race-checks?
2. We'd probably want to coordinate this change with a library rename. Something like core-location-client or core-location-dependency.

Going to keep this open for now while we find the time to figure out those details. Thanks for getting this work started though!

[3a4oT]

Hey, what is the status of this PR? What needs to be done to wrap up the new release?

[p4checo]

Given Dependencies define global instances, I think we need to ensure all dependencies' APIs support being called from multiple places of the application, possibly simultaneously/concurrently.

As it is, every time delegate is called, a new LocationManagerDelegate is created which will make any previous returned delegate stream not receive any new events. There's also the issue of CLLocationManager.delegate not being atomic on iOS, which can result in data races.

A possible solution for this could be to return a new stream for each caller, with the difference that all streams would share a single LocationManagerDelegate instance underneath that would be set as the single CLLocationManager.delegate (possibly when CLLocationManager is instantiated, avoiding data races). The LocationManagerDelegate could be made an actor managing a collection of streams (creation/deletion), forwarding delegate events accordingly.

Does this make sense? I can try to give this a go.

PS: Thanks for the awesome work 🙏🏼

[p4checo]

In the meantime, opened MR rcasula#2 which hopefully (also) addresses this issue 🤓

[p4checo]

For completeness' sake, following a discussion in this PF's slack thread, to ensure CLLocationManager is accessed from the main RunLoop an alternative solution could be to use a MainActorIsolated structure as introduced here.

It appears to ensure CLLocationManager's APIs are invoked on the Main RunLoop, so it has the benefit of avoiding the Task { @MainActor } and not requiring sprinkling @MainActor in all API closures, making it less verbose and possibly less error prone as well if more APIs are added in the future.

Seems like an interesting approach, but ideally someone with more experience in swift concurrency can chime in, or at least we have one more approach to consider 🤓

[p4checo]

@rcasula can this MR be marked as ready for review? What documentation updates are missing? Thanks!

[3a4oT]

Should it be enabled?

[p4checo]

If we do, SPM complains about unsafe flags being used when integrating it in other packages/projects

[3a4oT]

ohh, that's a good point. Just thinking loud here. What if we introduce ENV varible for local development and CI.
Something like :

if ProcessInfo.processInfo.environment["COMPOSABLE_DEVELOPER"] == "1" {
settings.append(.unsafeFlags([
     "-Xfrontend",
     "-warn-concurrency",
     "-Xfrontend",
     "-enable-actor-data-race-checks"
     ]))
}

Little incovinience to define variable locally but it can be documented on the README for developers.

[p4checo]

That's definitely an interesting idea! Let's see what the rest have to say 🤓

[3a4oT]

friendly ping, can we please get an feedback?

[3a4oT]

Hello, any chances to proceed with it?

[spiridonkopicl]

Thank you @rcasula, you saved my day 🍻