RxSwift to Combine translation #27

alejandro-ruiz-ts · 2021-06-30T10:01:09Z

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 from Cancellable instead of Disposable. Its dispose() method is replaced by the cancel() method.
Dispatcher.swift: the DispatcherSubscription class now inherits from Cancellable instead of Disposable. Its dispose() method is replaced by the cancel() method.
Middleware.swift: the Middleware logic remains the same, it does not depend on RxSwift.
ReducerGroup.swift: the GroupReducer protocol and its variable disposeBag will now be of type Cancellable. The disposeBag will be an array of Cancellables. The dispose() function is replaced by the cancel() function.
Service.swift: the Service protocol remains the same, it does not depend on RxSwift.
State.swift: the StateType protocol is now a simple basic protocol that allows us to differentiate and group the State classes of our application, nothing more. In Combine the State must be classes unlike before, which were Structs. This is so because only classes can inherit from ObservableObject, whose functionality thanks to Combine will now allow us to listen to state changes.
Store.swift: this class now consists of a State of type ObservableObject and a StoreController of type Cancellable. The ObservableObject 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 new Disposable of our application. The code of the class has been quite reduced and clear.

Regarding the rest of Mini's content:

Utils: prematurely, during the study of the original Mini to try to understand and know how everything works, we started by translating the operator extensions contained in 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.
MiniTasks & MiniPromises: these two modules are still pending to be translated into Combine, specifically their PrimitiveSequenceType extension. They are not strictly necessary for the architecture of a SwiftUI+Combine project. The code has been commented and marked as TODO.
TestMiddleware: remains the same as it does not depend on RxSwift.
LoggingService: remains the same as it does not depend on RxSwift.

Regarding the package.swift file for package configuration:

Dependencies and targets that referenced the RxSwift and RxOptional libraries have been removed.

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?

…

minuscorp

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

minuscorp · 2021-07-03T13:08:12Z

Sources/Mini/ActionReducer.swift

 /**
 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, *)


Remove call @available annotations and instead bump the minimum target version in the Package.swift file.

Fixed, now target version is 13.0.

minuscorp · 2021-07-03T13:09:51Z

Sources/Mini/Dispatcher.swift

@@ -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")


This must crash because data revving has been broken, if this happens, something about threading is wrong.

Ok, replaced. I like to control errors rather than cause crashes, but it doesn't seem to be a frequent error case.

minuscorp · 2021-07-03T13:12:02Z

Sources/Mini/Dispatcher.swift

@@ -100,6 +85,15 @@ public final class Dispatcher {
        )
        return registerInternal(subscription: subscription)
    }
+
+    private func getNewSubscriptionId() -> Int {


This method is not thread safe. Data races may occur and crash. Switch back to atomic operations if possible.

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.

minuscorp · 2021-07-03T13:15:03Z

Sources/Mini/ReducerGroup.swift

-public protocol Group: Disposable {
-    var disposeBag: CompositeDisposable { get }
+@available(iOS 13.0, *)
+public protocol GroupReducer: Cancellable {


Can be renamed to CancellableGroup for clarification.

minuscorp · 2021-07-03T13:16:33Z

Sources/Mini/StateType.swift

-
-import Foundation
-
-public protocol StateType {


Why deleting this protocol?

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.

minuscorp · 2021-07-03T13:18:20Z

Sources/Mini/Store.swift

-                }
-            }
+        didSet {
+            objectWillChange.send()


Only different states are propagated through the architecture, why the change?

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.

minuscorp · 2021-07-03T13:20:07Z

Sources/Mini/Store.swift

-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 {


State must conform to StateType, if it has to conform to ObservableObject, delegate it to the former protocol.

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.

Because working with protocols allow extensions to be done into generics while classes don't. It's a matter of extensibility.

Ok, protocol restored and applied at both State and Store.

minuscorp · 2021-07-03T13:21:08Z

Sources/Mini/Store.swift

-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 {


Also StoreType must be confirmed in the Store class, changes needed must be performed through the protocol.

Same here but with StoreType.

Ok, protocol restored and applied at both State and Store.

minuscorp · 2021-07-03T13:23:33Z

Sources/Mini/Utils/Extensions/ObservableType+Extensions.swift

+@available(iOS 13.0, *)
+public extension Publisher {
+
+    func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> {


Publishers.Filter<Self>?

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.

The return type of the function, wouldn't be more correct to be my suggestion?

Suggested change

func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> {

func filterKey(_ keyPath: KeyPath<Output, Bool>) -> Publishers.Filter<Self> {

minuscorp

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.

minuscorp · 2021-11-18T09:43:22Z

Package.swift

@@ -6,7 +6,7 @@ import PackageDescription
 let package = Package(
    name: "Mini",
    platforms: [
-        .iOS(.v11),
+        .iOS("13.0"),


I think it is time to bump the swift tools to 5.3 to avoid this string literal

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.

But you're not using atomic integers anymore don't you?

minuscorp · 2021-11-18T09:48:44Z

Sources/Mini/Store.swift

-}
-
-extension StoreType {
-    /**


The docs cannot disappear!

minuscorp · 2021-11-18T09:50:52Z

Sources/Mini/Utils/Extensions/ObservableType+Extensions.swift

+@available(iOS 13.0, *)
+public extension Publisher {
+
+    func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> {


Suggested change

func filterKey(_ keyPath: KeyPath<Output, Bool>) -> AnyPublisher<Output, Failure> {

func filterKey(_ keyPath: KeyPath<Output, Bool>) -> Publishers.Filter<Self> {

minuscorp · 2021-11-18T09:54:02Z

Sources/Mini/Utils/Extensions/ObservableType+Extensions.swift

+}
+
+
+public extension Publisher where Self.Output: OptionalType {


Maybe it'd be interesting to make an extension like: public extension Publisher where Self.Output: OptionalType, Self.Failure == Never

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

I think you need to set Failure == Never on the extension

minuscorp · 2021-11-20T13:21:03Z

Package.resolved

@@ -77,8 +77,8 @@
        "package": "Nimble",
        "repositoryURL": "https://github.com/Quick/Nimble",
        "state": {
-          "branch": "master",
-          "revision": "eea5843b34beb559dd51cf004953f75028e47add",
+          "branch": "main",


Let's lock into a version .upToNextMajor

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.

minuscorp · 2021-11-20T13:23:32Z

Sources/Mini/Dispatcher.swift

@@ -230,3 +231,26 @@ public final class DispatcherSubscription: Comparable, Disposable {
        return lhs.priority <= rhs.priority
    }
 }
+
+public class AtomicCounter {


This is something internal to the library, shouldn't be exposed as public

minuscorp · 2021-11-20T13:27:42Z

Sources/Mini/Dispatcher.swift

+
+public class AtomicCounter {
+
+    private var mutex = pthread_mutex_t()


os_unfair_lock_lock is the fastest

Ok, changed with this approach.

minuscorp · 2021-11-30T10:17:57Z

Package.swift

@@ -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"]


Suggested change

dependencies: ["NIOConcurrencyHelpers"]

dependencies: [.product(package: "swift-nio", package: "NIOConcurrencyHelpers"]

But again, if you're not using atomics anymore from swift-nio, you can delete the dependency

alejandro-ruiz-ts added 29 commits June 17, 2021 10:56

Test combine translation

6f058c2

Fix package.swift

46b7583

Add mini file

32d0659

Fix package.swift

93ea1a8

Fix package.swift

8294594

Remove promises

1212e10

Fix package.swift

faad2a2

Fix package.swift

af0a4a9

Fix package.swift

b09c565

Remove test middleware

7a90d24

Fix package.swift

864ac2b

Fix package.swift

f168731

Add iOS 13 annottation

15ae26f

Fix anotations

536b6af

Fix anotation

ad0cf43

Add iOS 13 support

09e9dae

Fix reducerGroup

4cbf493

ObservableType to Combine and Store dispatcher combine

e44ae39

Fix ObservableType

a556559

Fix withStateChanges method

e26870d

iOS 13 annotations added

445a5c9

First version Mini Combine

41049ef

Fix package.swift

b691d7a

Add anotation

0e769cf

Remove State protocol

44fa877

Fix StateType protocol for State classes

9569b17

Remove function from StateType protocol

5faac80

MiniPromises commented

2e32d42

Commented extensions in Promises

ef228c8

alejandro-ruiz-ts requested a review from minuscorp as a code owner June 30, 2021 10:01

alejandro-ruiz-ts requested review from r-pedraza and sebastianvarela as code owners June 30, 2021 10:01

minuscorp requested changes Jul 3, 2021

View reviewed changes

alejandro-ruiz-ts added 5 commits September 29, 2021 13:35

Fix var state type in Store

5a74f1f

Make public var state in Store

3206498

Fix PR comments.

0ad5774

Fix iOS version

f61c2b7

Fix iOS 13 deployment target

7caa67e

alejandro-ruiz-ts requested a review from minuscorp November 17, 2021 15:41

minuscorp requested changes Nov 20, 2021

View reviewed changes

alejandro-ruiz-ts added 4 commits November 29, 2021 14:36

Fix PR comments

4f57874

Fix Nimble version

f7aacc2

Fix SPM version to 5.2

9c027f1

Restore Swift tools version to 5.0

25fd602

alejandro-ruiz-ts force-pushed the combine-translation-test branch 6 times, most recently from b7e6762 to c422d27 Compare November 29, 2021 15:03

Restore SwiftTools to 5.0 version

0fe3ec3

alejandro-ruiz-ts force-pushed the combine-translation-test branch from c422d27 to 0fe3ec3 Compare November 29, 2021 15:04

minuscorp requested changes Nov 30, 2021

View reviewed changes

maca-technology closed this by deleting the head repository Oct 15, 2024

	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 {


		public class AtomicCounter {

		private var mutex = pthread_mutex_t()

	dependencies: ["NIOConcurrencyHelpers"]
	dependencies: [.product(package: "swift-nio", package: "NIOConcurrencyHelpers"]

RxSwift to Combine translation #27

RxSwift to Combine translation #27

Conversation

alejandro-ruiz-ts commented Jun 30, 2021

PR's key points

How to review this PR?

minuscorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-ruiz-ts Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minuscorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-ruiz-ts Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandro-ruiz-ts Nov 17, 2021 •

edited

Loading

alejandro-ruiz-ts Nov 29, 2021 •

edited

Loading