NoTests / RxFeedback.swift

The universal system operator and architecture for RxSwift
MIT License
1.04k stars 68 forks source link

[Proposal] - Make FeedbackLoop as a struct #11

Open sergdort opened 7 years ago

sergdort commented 7 years ago

Hi, guys!

I've been thinking how we can improve API in a way that users won't be able to path their own FeedbackLoop as they may use it wrong by not using scheduler correctly

So here is my proposal:

public struct FeedbackLoop<State, Event> {
    let loop: (ImmediateSchedulerType, Observable<State>) -> Observable<Event>

    public init<Control: Equatable>(query: @escaping (State) -> Control?, effects: @escaping (Control) -> Observable<Event>) {
        self.loop = { scheduler, state -> Observable<Event> in
            return state.map(query)
                .distinctUntilChanged { $0 == $1 }
                .flatMapLatest { control -> Observable<Event> in
                    guard let control = control else { return Observable.empty() }

                    return effects(control).enqueue(scheduler)
                }
        }
    }

    public init(predicate: @escaping (State) -> Bool, effects: @escaping (State) -> Observable<Event>) {
        self.loop = { scheduler, state -> Observable<Event> in
            return state.flatMapLatest { state -> Observable<Event> in
                guard predicate(state) else { return Observable.empty() }

                return effects(state).enqueue(scheduler)
            }
        }
    }

    public init(effects: @escaping (State) -> Observable<Event>) {
        self.loop = { scheduler, state in
            return state.flatMapLatest { state in
                return effects(state).enqueue(scheduler)
            }
        }
    }
}

By exposing only these three available initializers. I guess we can also somehow figure out how something similar can be adopted for Driver as well.

What do you guys think?

kzaher commented 7 years ago

Hi @sergdort ,

How would we extend it? It seems to me that only way would be to expose loop lambda, but if we do that, then it's still possible to mess up.

I guess this would also maybe reduce chances of error.

public enum FeedbackLoop<State, Event> {
    case startRequest(query: (State) -> Control?, effects: (Control) -> Observable<Event>)
    ...
    case custom((ImmediateSchedulerType, Observable<State>) -> Observable<Event>)
}

extension FeedbackLoop {
     var loop: (ImmediateSchedulerType, Observable<State>) -> Observable<Event>
}

That would allow us to give names to most common feedback loops.

sergdort commented 7 years ago

It seems to me that only way would be to expose loop lambda, but if we do that, then it's still possible to mess up.

I'm a little bit confused :) The idea is to not expose loop closure but only expose initializers

kzaher commented 7 years ago

@sergdort yeah, I understand that, but how would somebody create an arbitrary feedback loop if needed? Don't think there is a finite number of them.

sergdort commented 7 years ago

Do you think this one is not enough? Or you think someone would want to use something else then flatMapLatest ?

public init(effects: @escaping (State) -> Observable<Event>) {
        self.loop = { scheduler, state in
            return state.flatMapLatest { state in
                return effects(state).enqueue(scheduler)
            }
        }
    }
kzaher commented 7 years ago

I don't think it's just a matter of customizing flatMapLatest.

E.g. there is this form that uses a set that controls it:

public func react<State, Control: Hashable, Event>(
    query: @escaping (State) -> Set<Control>,
    effects: @escaping (Control) -> Driver<Event>
    ) -> (Driver<State>) -> Driver<Event> {
    return { state in
        let query = state.map(query)

        let newQueries = Driver.zip(query, query.startWith(Set())) { $0.subtracting($1) }

        return newQueries.flatMap { controls in
            return Driver.merge(controls.map { control -> Driver<Event> in
                return query.filter { !$0.contains(control) }
                    .map { _ in Driver<Event>.empty() }
                    .startWith(effects(control).enqueue())
                    .switchLatest()
            })
        }
    }
}

I'm assuming one could potentially want concat in case events represents some form of commands that mutate state and can't be cancelled vs queries.

It's hard to anticipate all of the possible combinations :), I'm not sure we should assume there is a finite number of them.

sergdort commented 7 years ago

Oh, I see :)

Anyway, isn't the difference only in "flatting" strategy? Can't think of anything else.

sergdort commented 7 years ago

Ignore me =) I just read code snipped above one more time...

kzaher commented 7 years ago

Actually, the flattening strategy here is the same ;)

kzaher commented 7 years ago

I guess we can probably improve them in some way ...