Conveniences for creating state machine as Property.

[andersio]

1. Introduce two conveniences that yield a Property state machine directly.
   Updated the examples to demonstrate them.

2. Remove SignalProducer.deferred.

"Property conveniences" turn out to be not so convenient in terms of implementation, if we are to abide with the initial value can only be seen once expectation around Property.

So I have pulled pieces of #10 and managed to find a solution that can back both SignalProducer and Property based APIs.

This PR is marked as blocked though, since it is affected by a freshly identified SignalProducer bug in ReactiveSwift & the test case would fail due to this. It seems somehow I circumvented it...

[sergdort]

No need atomic, right?

[RuiAAPeres]

What's the advantage of this approach, versus the original spec and for what we do internally?

[andersio]

@RuiAAPeres Across all examples:

1. https://github.com/Babylonpartners/ReactiveFeedback/blob/develop/Example/ViewController.swift#L64
2. https://github.com/Babylonpartners/ReactiveFeedback/blob/develop/Example/PaginationViewController.swift#L97
3. https://github.com/Babylonpartners/babylon-ios/pull/3068/files#diff-bad887f7d560694d6c4ab41c3970b545R121
4. https://github.com/Babylonpartners/babylon-ios/pull/3019/files#diff-efea1ffa804e8a2a1aa4d40eda780c8aR34

You can observe that because we are exposing properties as public VM APIs for VC bindings, we need the ability to replay the latest state to back these properties up. Especially for Forms (as shown in item 3), form builder layouts are computed as a function of state.

So having conveniences to create Property backed state machine is driven by these use cases.

As for being synchronous by default, it is more of an implementation tweak. Opt-in asynchrony reduces surprises, and is generally easier to reason about. It also aligns with ReactiveSwift, which is synchronous unless one having explicitly requested.

To allow feedback to propagate synchronously, a buffer is introduced for flattening recursively delivered Events into an iterative queue drain. Without the buffer the state machine would deadlocks due to recursive value delivery not being permitted in Signal and MutableProperty.

versus the original spec?

This does not deviate from the original proposal, except now that a Scheduler is no longer mandatory for creating the state machine, and users can now assume the state machine is safely synchronous.

[sergdort]

don't we need to add Disposable to lifetime

[andersio]

Did on line 37.

startWithSignal does not return a Disposable. What it does is:

1. make a Signal.
2. run the given (Signal, Disposable) -> Void closure.
3. start the deferred work.

[sergdort]

TBH I still not convinced with this approach

1. I feel it's way less elegant then current solution just looking on:

        return SignalProducer.deferred {
            let (state, observer) = Signal<Value, NoError>.pipe()

            let events = feedbacks.map { feedback in
                return feedback.events(scheduler, state)
            }

            return SignalProducer<Event, NoError>(Signal.merge(events))
                .scan(initial, reduce)
                .prefix(value: initial)
                .on(value: observer.send(value:))
        }

Vs

return SignalProducer { observer, lifetime in
            let (events, eventsObserver) = Signal<Event, NoError>.pipe()
            lifetime += AnyDisposable(eventsObserver.sendInterrupted)

            StateMachine.bootstrap(
                state: SignalProducer<Event, NoError>(events)
                    .scan(initial, reduce)
                    .prefix(value: initial)
                    .on(value: observer.send(value:)),
                process: eventsObserver.send(value:),
                feedbacks: feedbacks,
                lifetime: lifetime
            )
        }


internal enum StateMachine {
    internal static func bootstrap<State, Event>(
        state: SignalProducer<State, NoError>,
        process: @escaping (Event) -> Void,
        feedbacks: [Feedback<State, Event>],
        lifetime: Lifetime
    ) {
        // Treat initialisation as an implicit event, and buffer any event yielded during
        // the initialisation.
        let buffer = Atomic(EventBuffer<Event>(isTransitioning: true, events: []))

        state.startWithSignal { state, interruptHandle in
            lifetime += interruptHandle

            let events = feedbacks.map { feedback in
                return feedback.events(ImmediateScheduler(), state)
            }

            lifetime += Signal.merge(events)
                .observeValues { event in
                    let shouldProceed: Bool = buffer.modify { buffer in
                        guard buffer.isTransitioning else {
                            // This event is not recursively sent during the processing
                            // of another event.
                            buffer.isTransitioning = true
                            return true
                        }

                        buffer.events.append(event)
                        return false
                    }

                    guard shouldProceed else { return }

                    process(event)

                    // Drain any event recursively yielded during `process(event)` above.
                    while let event = buffer.modify({ $0.completeOrDequeue() }) {
                        process(event)
                    }
                }
        }

        // Drain any event yielded by the initial state.
        while let event = buffer.modify({ $0.completeOrDequeue() }) {
            process(event)
        }
    }
}

2. If one would want to use a Property with current approach would just need to Property(initial: value, then: state) Not a big overhead IMHO.

3. Being Synchronous is not an that big of advantage compared to introduced complexity. It introduces an unnecessary gap in the level of abstractions, I mean instead of using declarative nature of ReactiveSwift library we are going down t the level of Atomic and introduce StateMachine .

What I would like to propose is to stop premature optimisation, and focus on shaping naming issues, add documentation, unit tests and maybe add more examples

[sergdort]

Sorry, but I'm not convinced with this approach ¯_(ツ)_/¯

[RuiAAPeres]

So having conveniences to create Property backed state machine is driven by these use cases.

Don't you get this from the initial state passed to the reducer + Producer we have with the initial draft?

Without the buffer the state machine would deadlocks due to recursive value delivery not being permitted in Signal and MutableProperty

Do you have a test case for this?

This does not deviate from the original proposal, except now that a Scheduler is no longer mandatory for creating the state machine, and users can now assume the state machine is safely synchronous.

This is an oxymoron. There's a clear deviation, at least from an implementation POV.

[RuiAAPeres]

I agree with this:

What I would like to propose is to stop premature optimisation, and focus on shaping naming issues, add documentation, unit tests and maybe more examples

[andersio]

Stripped all the premature optimisation away. :)

[RuiAAPeres]

LGTM. 👍

[sergdort]

Why you don't like deferred? =)
It's the same but it looks much cleaner IMO

[RuiAAPeres]

Please fix conflicts, so it can be merged.