-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor to Allow Explicit Inputs. #32
Conversation
152debb
to
bdcb8b8
Compare
@zzcgumn I had a similar issue once when I was trying to make a state to react on some external signal. But thinking of it now can we achieve the same by creating a regular class ViewModel {
init(externalSignal: Signal<ExternalSignalValue, NoError>) {
...
state = Property(
initial: .initial,
reduce: ViewModel.reduce,
feedbacks: [
ViewModel
.userActions(actions: actions),
...,
ViewModel
.externalSignal(externalSignal),
)
}
}
extension ViewModel {
static func externalSignal(_ signal: Signal<ExternalSignalValue, NoError>) -> Feedback<State, Event> {
return Feedback { _ in
return signal.map(Event.external)
}
}
}
enum Event {
case ui
...
case external(ExternalSignalValue)
} |
While I agree on the need of convenience for turning external reactive sources into |
Hi gentlemen. 👋 There are two parts to this proposal and we should tackle them separately and consequently in the right order and place.
I want to make this very clear: even if this PR is approved and merged by no means it follows that we will adopt in our codebase without proper discussion and agreement. 😄 |
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'm +1 for allowing explicit external inputs (which will solve initial racing issue discussed in #38 ) , but without using different reducers provided by FeedbackInputSignal
.
Ideally:
// Use plain `Signal` for external input rather than `FeedbackInputSignal`.
let inputs = Signal.merge(
nearBottomSignal.map { .startLoadingNextPage },
// NOTE: { !$0.paging } filtering should be done in existing `reduce`
retrySignal.map { .retry },
)
let system: SignalProducer<State, NoError> = .system(input: inputs,
initial: State.initial,
scheduler: UIScheduler(),
reduce: State.reduce,
feedbacks: feedbacks)
The reason behind is, separating reducers (State -> Event -> State
and State -> Event -> Event?
) means we will switch from current Moore state machine model to Mealy (EDIT: #32 (comment)) , which will heavily impact existing state transition diagram (Moore tends to have more clumsy states than Mealy).
- Mealy:
(reducer: State -> Event -> State, output: State -> Event -> Output
) - Moore: (reducer:
State -> Event -> State
, output:State -> Output
)
If we want to switch to Mealy model, I highly recommend to check out my ReactiveAutomaton and keep this library at Moore-level as is.
@inamiy what do you mean by separating reducers? If I understand correctly the way input signal would work is that it would map input values to some events which then will still go through the common reducer. |
According to this I would say we have |
Anyway, the definition of reducer is not a problem here, but
|
I think I was a bit wrong about relating My whole point is, And all the state-filtering logic needs to go into "common reducer" as described in #32 (review) . |
@inamiy Does this signature |
Isn't Also, correct me if I'm wrong, I have a feeling that you wan't something to be able to trigger effects, e.g |
@zzcgumn can we reach an outcome for this? Otherwise, I am afraid this needs to be closed. |
@RuiAAPeres I will have a careful look at what @inamiy'a suggestion tomorrow morning. At a first glance what he suggests makes sense. |
@zzcgumn awesome! Do please remember that merging this work/approach doesn't mean we will adopt it. |
(Sorry for late response, seems like my prev reply hasn't been successfully sent 😢)
Current
Yes, so I introduced an example in #41 😉 |
I'm with @andersio the issue I see here is that #41 introduce a set of events that does nothing to the The signature of |
FYI: To support opinions above, cf. Labeled transition systems https://ncatlab.org/nlab/show/transition+system
Let |
To have a
Intuitively, I would expect 1 to be dominating use case. I think we can have situations when 2 makes it easier to build highly dynamic user interfaces. Scenario 3, we should probably avoid. It is essentially my original proposal. What I was worried about was that it might not be possible to calculate @inamiy Does the above agree with how you are reasoning? P.S. Am I completely wrong to think of a transition system as a Markov chain with deterministic transitions? |
Isn't that effectively |
@ilyapuchka is right. So instead, I would like to propose a new feedback system that depends on both It will be based on #32 (comment) and #41 , and I will post to ios-playbook later :) |
Please re-open when you can work on it. 👍 |
Current production code tend to subscribe to external inputs in a way that is slightly awkward. In this example from the demo app it is quite difficult to reason about when the
startLoadingNextPage
event will fire.I propose that we resolve this by adding explicit inputs which listen to an external input. When an input is received they evaluate a function
(State, InputEvent) -> Event?
which models if and how the system should react to anInputEvent
given its currentState
.This code would need a bit of tidying up before being merged, but I would appreciate feedback on whether explicit inputs is considered to be a good design and if the proposed implementation can be improved.
❤️
@zzcgumn