-
Notifications
You must be signed in to change notification settings - Fork 5
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
RxSwift to Combine translation #27
RxSwift to Combine translation #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, this can be indeed used from the branch but not ready for merging, there's functionality left to be translated to bridge with previous' library versions.
Sources/Mini/ActionReducer.swift
Outdated
/** | ||
The `Reducer` defines the behavior to be executed when a certain | ||
`Action` object is received. | ||
*/ | ||
public class Reducer<A: Action>: Disposable { | ||
@available(iOS 13.0, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove call @available
annotations and instead bump the minimum target version in the Package.swift
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, now target version is 13.0.
Sources/Mini/Dispatcher.swift
Outdated
@@ -133,15 +127,29 @@ public final class Dispatcher { | |||
if let action = object as? T { | |||
completion(action) | |||
} else { | |||
fatalError("Casting to \(tag) failed") | |||
print("MiniError: Casting to \(tag) failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must crash because data revving has been broken, if this happens, something about threading is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, replaced. I like to control errors rather than cause crashes, but it doesn't seem to be a frequent error case.
@@ -100,6 +85,15 @@ public final class Dispatcher { | |||
) | |||
return registerInternal(subscription: subscription) | |||
} | |||
|
|||
private func getNewSubscriptionId() -> Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not thread safe. Data races may occur and crash. Switch back to atomic operations if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have added an Atomic class to control this value in a safe way. It should be similar to the way you had it implemented before.
Sources/Mini/ReducerGroup.swift
Outdated
public protocol Group: Disposable { | ||
var disposeBag: CompositeDisposable { get } | ||
@available(iOS 13.0, *) | ||
public protocol GroupReducer: Cancellable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be renamed to CancellableGroup
for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed!
|
||
import Foundation | ||
|
||
public protocol StateType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why deleting this protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not seen it necessary at any point in the flow, in fact I could delete it completely but I leave it in case in Rx it makes more sense.
Sources/Mini/Store.swift
Outdated
} | ||
} | ||
didSet { | ||
objectWillChange.send() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only different states are propagated through the architecture, why the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now state is an @published variable that allows us to listen when the whole state changes or when any of its parameters change individually. This works quite well for us from Combine, it is much simpler than before.
Sources/Mini/Store.swift
Outdated
public class Store<State: StateType, StoreController: Disposable>: ObservableType, StoreType { | ||
public typealias Element = State | ||
@available(iOS 13.0, *) | ||
public class Store<State: ObservableObject, StoreController: Cancellable>: ObservableObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State
must conform to StateType
, if it has to conform to ObservableObject
, delegate it to the former protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the need for the StateType and its implication in these classes. Right now in our flow it seems irrelevant. I would appreciate if you could tell me a bit more about this and see if I understand why to include it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because working with protocols allow extensions to be done into generics while classes don't. It's a matter of extensibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, protocol restored and applied at both State and Store.
Sources/Mini/Store.swift
Outdated
public class Store<State: StateType, StoreController: Disposable>: ObservableType, StoreType { | ||
public typealias Element = State | ||
@available(iOS 13.0, *) | ||
public class Store<State: ObservableObject, StoreController: Cancellable>: ObservableObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also StoreType
must be confirmed in the Store
class, changes needed must be performed through the protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here but with StoreType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, protocol restored and applied at both State and Store.
@available(iOS 13.0, *) | ||
public extension Publisher { | ||
|
||
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publishers.Filter<Self>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extension of operators I have been very close to delete it all, since in Combine none of them are needed, it has its own native operators. But I understand that for use in Rx it may still make sense to keep them, but they are irrelevant for us, they would not be needed. If you want me to adapt or change any of this, let me know exactly what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of the function, wouldn't be more correct to be my suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> { | |
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> Publishers.Filter<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, we cannot ensure that this library will be used only in SwiftUI, but in UIKit, so there're still things to do and translate to keep the old implementation removing just RxSwift dependency.
@@ -6,7 +6,7 @@ import PackageDescription | |||
let package = Package( | |||
name: "Mini", | |||
platforms: [ | |||
.iOS(.v11), | |||
.iOS("13.0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is time to bump the swift tools to 5.3 to avoid this string literal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried it but it gives me error because it does not find the target: NIOConcurrencyHelpers. I have left it in the 5.0 version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you're not using atomic integers anymore don't you?
} | ||
|
||
extension StoreType { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs cannot disappear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Fixed.
@available(iOS 13.0, *) | ||
public extension Publisher { | ||
|
||
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> { | |
func filterKey(_ keyPath: KeyPath<Output, Bool>) -> Publishers.Filter<Self> { |
} | ||
|
||
|
||
public extension Publisher where Self.Output: OptionalType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be interesting to make an extension like: public extension Publisher where Self.Output: OptionalType, Self.Failure == Never
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First filter changed but second comment, about extension, causes this function to stop working:
func select<T: OptionalType>(_ keyPath: KeyPath<Output, T>) -> AnyPublisher<T.Wrapped, Self.Failure> where T.Wrapped: Equatable {
map(keyPath)
.filterNil()
.removeDuplicates()
.eraseToAnyPublisher()
}
This error: Referencing instance method 'filterNil()' on 'Publisher' requires the types 'Publishers.MapKeyPath<Self, T>.Failure' (aka 'Self.Failure') and 'Never' be equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to set Failure == Never on the extension
@@ -77,8 +77,8 @@ | |||
"package": "Nimble", | |||
"repositoryURL": "https://github.com/Quick/Nimble", | |||
"state": { | |||
"branch": "master", | |||
"revision": "eea5843b34beb559dd51cf004953f75028e47add", | |||
"branch": "main", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's lock into a version .upToNextMajor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried it but it gives me error because it does not find the library: NIOConcurrencyHelpers
. I have left it in the main branch in the meantime.
Sources/Mini/Dispatcher.swift
Outdated
@@ -230,3 +231,26 @@ public final class DispatcherSubscription: Comparable, Disposable { | |||
return lhs.priority <= rhs.priority | |||
} | |||
} | |||
|
|||
public class AtomicCounter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something internal to the library, shouldn't be exposed as public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
Sources/Mini/Dispatcher.swift
Outdated
|
||
public class AtomicCounter { | ||
|
||
private var mutex = pthread_mutex_t() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os_unfair_lock_lock
is the fastest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed with this approach.
b7e6762
to
c422d27
Compare
c422d27
to
0fe3ec3
Compare
@@ -56,7 +54,7 @@ let package = Package( | |||
// Targets can depend on other targets in this package, and on products in packages which this package depends on. | |||
.target( | |||
name: "Mini", | |||
dependencies: ["RxSwift", "NIOConcurrencyHelpers", "RxOptional"] | |||
dependencies: ["NIOConcurrencyHelpers"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependencies: ["NIOConcurrencyHelpers"] | |
dependencies: [.product(package: "swift-nio", package: "NIOConcurrencyHelpers"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But again, if you're not using atomics anymore from swift-nio, you can delete the dependency
PR's key points
RxSwift to Combine translation of the FLUX architecture contained in Mini. The goal is to implement a Mini with the native reactive iOS library called Combine that can be used in a native reactive application with SwiftUI (or even with UIKit). To do this, these changes have been made to the following Mini core files:
Action.swift
: the Action protocol remains the same, it does not depend on RxSwift.ActionReducer.swift
: the Reducer class will now inherit fromCancellable
instead ofDisposable
. Its dispose() method is replaced by the cancel() method.Dispatcher.swift
: theDispatcherSubscription
class now inherits fromCancellable
instead ofDisposable
. Itsdispose()
method is replaced by thecancel()
method.Middleware.swift
: theMiddleware
logic remains the same, it does not depend on RxSwift.ReducerGroup.swift
: theGroupReducer
protocol and its variabledisposeBag
will now be of typeCancellable
. The disposeBag will be an array ofCancellables
. Thedispose()
function is replaced by thecancel()
function.Service.swift
: theService
protocol remains the same, it does not depend on RxSwift.State.swift
: theStateType
protocol is now a simple basic protocol that allows us to differentiate and group theState
classes of our application, nothing more. In Combine theState
must beclasses
unlike before, which wereStructs
. This is so because only classes can inherit fromObservableObject
, whose functionality thanks to Combine will now allow us to listen to state changes.Store.swift
: this class now consists of aState
of typeObservableObject
and aStoreController
of typeCancellable
. TheObservableObject
of Combine will be the one that will allow us to subscribe to the changes that occur in that state. This allows us to get rid of all the extensions and functions dedicated to listen to the states that we had when we depended on RxSwift. In the same way,Cancellable
will be the newDisposable
of our application. The code of the class has been quite reduced and clear.Regarding the rest of Mini's content:
ObservableType+Extensions.swift
from RxSwift to Combine. Although they are not ultimately needed within a project that uses Combine+SwiftUI, the native library provides the same operators. All other classes in this directory remain the same, they do not depend on RxSwift.PrimitiveSequenceType
extension. They are not strictly necessary for the architecture of a SwiftUI+Combine project. The code has been commented and marked as TODO.Regarding the
package.swift
file for package configuration:This new MiniCombine has been tested and verified in a project with FLUX architecture using the Assembler and dispatching actions to the stores. Its performance is correct and similar to what we had with MiniRxSwift.
How to review this PR?
…