search

steamclock / netable

A Swift library for encapsulating network APIs using Codable in a type-oriented way.
MIT License
99 stars 3 forks source link

Add Sendable conformance #116

Closed amyoulton closed 1 year ago

amyoulton commented 1 year ago

This PR adds Sendable conformance to both Netable itself, as well as the example project.

Netable

The changes in Netable were mostly simple, with a few caveats. Every public store property within Netable has to conform to Sendable, as it's an actor. The most complicated of them is AnyPublisher and I have left a comment directly on that piece of code. In general, the Combine framework and Sendable are not a super big fan of one another. I'll discuss how this was mostly navigated lower down in the PR, but AnyPublisher threw the biggest wrench for sure.

We also previously discussed removing Sendable conformance from the requestFailureDelegate because it doesn't need to be Sendable given how it operates, but it does need to be Sendable given that it's a stored property within the Netable actor.

Example Project

Mostly of the issues were solved with declaring the code run on the MainActor, as discussed in this slack thread.

The only "odd ball" is the error service. I typically wouldn't want any service to be forced to run on the main thread, but given the nature of the error service in our case, I think it makes sense.

brendanlensink commented 1 year ago

It looks like we've still got a whole bunch of Concurrency related warnings when building this project.

Are we able to resolve some of these, or is there some reason we can't deal with them?

amyoulton commented 1 year ago

The main project and both the Example project and Tests targets have complete currency checking selected but the netable target was apparently only marked minimal. I have changed this over and will fix this warnings.

amyoulton commented 1 year ago

@brendanlensink alright, everything is updated with the exception of two persistent warnings. After a conversation with @nbrooke in slack, it seems these are inherently not concurrency safe, but based off the code, is fine to leave.

For the first error, there is not great way to ignore the warning beyond marking the entire class an @unchecked which we don't want to do. The majority of recommend solutions suggest to decrease the concurrency checking 🥴

Given this, I think leaving these two is our final choice!