rizumita / Selm

Selm is the framework to realize Elm architecture in Swift
MIT License
13 stars 1 forks source link

Feature/Task #4

Closed kylelol closed 4 years ago

kylelol commented 4 years ago

Added in a basic implementation of the Task model, modeled from the Elm core library.
Also updated the sample project to have a new button to trigger the update using a Task.

TODO:

rizumita commented 4 years ago

@kylelol Thank you for your PR! How about forcing the Task to own an error type explicitly?

Task<Value, Failure>
rizumita commented 4 years ago

@kylelol I want Task to provide a set of AnyCancellable for Combine publisher.

How do you think about the following code?

class Ref<T> {
    private var value: T

    init(_ value: inout T) {
        self.value = value
    }
}

public struct Task<Value> {
...
    let refCancellables: Ref<Set<AnyCancellable>>

    public init(workWithCancellables: @escaping (@escaping Observer, inout Set<AnyCancellable>) -> Void) {
        var cancellables = Set<AnyCancellable>()
        self.refCancellables = Ref(&cancellables)
        self.work = { fulfill in
            workWithCancellables(fulfill, &cancellables)
        }
    }
...
}

public struct Cmd<Msg> {
...
    public static func ofTask<Value>(mapResult: @escaping (Result<Value, Error>) -> Msg, task: Task<Value>) -> Cmd<Msg> {
        return Cmd(value: [ { dispatch in
            task.work { result in
                _ = task
                let msg = mapResult(result)
                DispatchQueue.main.async { dispatch(msg) }
            }
        }])
    }
}
kylelol commented 4 years ago

@rizumita I definitely wan't task to be extended to work with combine.
Do you think we need the Ref type to wrap up the Set<AnyCancellable>? What would be the implications of not wrapping it?
Also why do we have to do _ = task inside the ofTask function?

rizumita commented 4 years ago

@kylelol Thanks for supporting the error type!

First, The Ref code was wrong. The right code is the following.

class Ref<T> {
    private var ptr: UnsafePointer<T>!
    var value: T { return ptr.pointee }

    init(_ value: inout T) {
        withUnsafePointer(to: &value) { ptr = $0 }
    }
}

The Task has to own reference to the set of AnyCancellable because the Set type is 'struct.' If it holds the set through without the Ref and closure doesn't own the Task, the set is released, and the sinking of Publisher is canceled immediately.

rizumita commented 4 years ago

@kylelol This code may be more easier than the Ref.

class CancellablesHolder {
    var cancellables = Set<AnyCancellable>()
}

public struct Task<Value> {
...
    let cancellablesHolder = CancellablesHolder()

    public init(workWithCancellables: @escaping (@escaping Observer, inout Set<AnyCancellable>) -> Void) {
        let holder = cancellablesHolder
        self.work = { fulfill in
            workWithCancellables(fulfill, &holder.cancellables)
        }
    }
...
}
kylelol commented 4 years ago

@rizumita You were right, I tried this:

public struct Task<Value> {
...
    let cancellables: Set<AnyCancellable>

    public init(workWithCancellables: @escaping (@escaping Observer, inout Set<AnyCancellable>) -> Void) {
        var cancellables = Set<AnyCancellable>()
        self.cancellables = cancellables
        self.work = { fulfill in
            workWithCancellables(fulfill, &cancellables)
        }
    }
...
}

public static func ofTask<Value, ErrorType: Swift.Error>(mapResult: @escaping (Result<Value, ErrorType>) -> Msg, task: Task<Value, ErrorType>) -> Cmd<Msg> {
        return Cmd(value: [ { dispatch in
            task.work { result in
                _ = task
                print(task.cancellables) // output was empty array
                let msg = mapResult(result)
                DispatchQueue.main.async { dispatch(msg) }
            }
        }])
    }

And the array was empty when I printed it out in the ofTask function. However the sink was still firing my timer. I'm guessing it's because of the _ = task line which keeps the task around.

I will update the code to use the CancellablesHolder class as a private implementation, and if we start having to do this in more places we can abstract it out.

rizumita commented 4 years ago

@kylelol I think PR became excellent. Thank you!

I was worried about the place of the set of cancellable. By the PR, I became not to need to care about the problem.

rizumita commented 4 years ago

@kylelol Do you have any change plan? Can I merge the PR?

kylelol commented 4 years ago

@rizumita Not at the moment. I think Task is in a good spot for now.
I wonder if for the future, Selm should depend on the Swiftz package, that way we can utilized some of the types like Monad (since Task is basically a Monad)

rizumita commented 4 years ago

@kylelol

Not at the moment.

Does this mean that I can merge?

kylelol commented 4 years ago

@rizumita yes please merge it!