Most general form of feedback loops #7
Conversation
- Implemented initial version of AsyncSynchronized to synchronize state mutation.
- To do `areEqual` parameter is not needed anymore in Swift but we might still want in Kotlin. This should be discussed further with Zaher and Juraj. - Changed the other `react` methods to use the feedback loops as a general
|
Nice! Thanks for contributing.
I went through the PR and I’m having hard time understanding what’s going
on. Namings like `Lifetime...` are not intuitive to me. Could you please
add some comments explaining what’s happening? For example, class comments
for the `RequestLifetimeTracking` and its nested classes.
Also, would love to get rid of the optional guy. In my apps I use the
Arrow’s optional. This generalization will help delete our version. Even if
we don’t manage to delete it, I’d like to make it internal and expose an
interface with standard kotlin types.
Thanks,
Marko
On Sun, 5 May 2019 at 16:44, eliekarouz ***@***.***> wrote:
This pull requests includes the most general form of feedback loops that
was implemented on RxFeedback.swift. TODO:
-
Unit Tests
This is still a draft as I still need to add the unit tests for the
new feedback loops. I am not very familiar with unit testing with
TestScheduler. I will be doing some research to understand how this
works.
-
Equatable
In the Swift version of this library, we are forcing the Query to be
equatable. There was an overload that takes areEqual: (Query, Query)
-> Bool. This was removed from RxFeedback.swift . @kzaher
<https://github.com/kzaher> I think this was needed for arrays because
prior Swift 4.2 to handle cases like these: Array<Equatable> was not
Equatable. but I think we still need in Kotlin because we need to call
special methods for deep comparison of arrays.
We might need to add an overload to the most general form that takes a (Query,
Query) -> Boolean parameter.
-
Optional<T>
Regarding the Optional type. I am not sure but I don't think this is
needed. Maybe we can provide a new API without Optional and deprecate the
current ones that use Optional. We can in a future version decide to remove
them.
We can keep though the Optional type as it can be used to wrap Kotlin
nullables when we need to have to build streams with nullable values.
------------------------------
You can view, comment on, or merge this pull request online at:
#7
Commit Summary
- Most general form of FLs:
- - Implemented the most general form of feedback loops.
- Better handling of `areEqual`.
- Added `reactSafe` for `Equatable`
File Changes
- *A*
rxfeedback/src/main/java/org/notests/rxfeedback/AsyncSynchronized.kt
<https://github.com/NoTests/RxFeedback.kt/pull/7/files#diff-0> (36)
- *M* rxfeedback/src/main/java/org/notests/rxfeedback/Feedbacks.kt
<https://github.com/NoTests/RxFeedback.kt/pull/7/files#diff-1> (272)
Patch Links:
- https://github.com/NoTests/RxFeedback.kt/pull/7.patch
- https://github.com/NoTests/RxFeedback.kt/pull/7.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABTW25PHCHFLVT2H7BNTYO3PT3XDVANCNFSM4HK3NCQA>
.
--
Pozdrav,
Marko
|
|
@eliekarouz yes, that was because of compiler limitations :(. |
|
@markotron I've refactored the feedback loops in Swift version so that they are all just simplifications of one general form. Seemed elegant to me. The idea is to have a set of identifiable values controlling the lifetime of observable state machines. When a new value is added to a set, new observable state machine in effects is subscribed to. When it is removed the subscription is disposed/unsubscribed -> effectively killing the state machine inside the observable. If the value for some key changes that is sent to the We've renamed |
|
I see. It does seem more elegant.
I’m still unfamiliar with some terminology that you use like: *observable
state machine, identifiable values *(is that just an object that can be
hashed?).
I get the general idea, I’ll take a look tmrw morning again and ask more
things probably :P
On Wed, 8 May 2019 at 20:37, Krunoslav Zaher ***@***.***> wrote:
@markotron <https://github.com/markotron> I've refactored the feedback
loops in Swift version so that they are all just simplifications of one
general form. Seemed elegant to me.
The idea is to have a set of identifiable values controlling the lifetime
of observable state machines. When a new value is added to a set, new
observable state machine in effects is subscribed to. When it is removed
the subscription is disposed/unsubscribed -> effectively killing the state
machine inside the observable.
If the value for some key changes that is sent to the state parameter
inside (intialQuery: Query, state: Observable<Query>) -> Observable<Event>
.
We've renamed Query to Request. People were complaining about the name
Query. I think it might make sense to rename to Kotlin version also.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABTW25PFSCY7PO7JWXC7HVLPUMMVNANCNFSM4HK3NCQA>
.
--
Pozdrav,
Marko
|
|
observable state machine -> Observable |
Sure. Will work on it this weekend... |
| queryLifetime.latestQuery.onNext(query) | ||
| } else continue | ||
| } else { | ||
| val subscription = CompositeDisposable() // SingleAssignmentDisposable |
There was a problem hiding this comment.
Instead of using a CompositeDisposable you could have rearranged this so that you add a new QueryID to the lifetimeByIdentifier map after subscribing to the effect, right? In this case, you don't need a ComponsiteDisposable that will actually contain just one Disposable.
There was a problem hiding this comment.
Yeah you are right... I was not very convinced of what I was writing there...
There was a problem hiding this comment.
Also, does the
AsyncSynchronizedconcept have a name? Is it used somewere else as well? I'd like to read more about it.
Thanks for the feedback... Maybe SerialQueue? I don't think there is a standard definition for for this concept because the mutations can run on any thread vs serial execution queues which is a know concept on iOS. With serial execution queues all mutations are dispatched to the same thread. Not sure if @kzaher has any link to share with us...
There was a problem hiding this comment.
@markotron , I think it's called AyncLock
| * as well as update already activated effects with the new Query that was returned. | ||
| */ | ||
| private class QueryLifetimeTracking<Query, QueryID, Event>( | ||
| val effects: (intialQuery: Query, state: Observable<Query>) -> Observable<Event>, |
There was a problem hiding this comment.
Now the effects accepts the state: Observable<Query> as a parameter, but in all our current helpers we're just ignoring this parameter:
effects = { initial: Query, _ ->
effects(initial)
}What is the use case of this state parameter?
There was a problem hiding this comment.
The idea of the general form is to have some standard way to implement nested systems while still updating the inner systems with the parent substate/state changes.
The react and reactSet can be seen as a projection (subset) of the general form. It would be better to provide the general form but also refactor existing react methods to use the general form in order to reduce the code base...
There was a problem hiding this comment.
Yeah, I understand why we have a general form. What I meant is that the general form is more general then all our current use cases (actually, my use cases :)). I thought that the implementation in swift was created with a specific use case in mind and wanted to know what that use case is. @kzaher did you use this general form for something that cannot be described without a general form?
|
|
||
| private val queue = mutableListOf<Mutate<State>>() | ||
|
|
||
| fun async(mutate: (State) -> Unit) { |
There was a problem hiding this comment.
Should this be named async? :)
There was a problem hiding this comment.
@markotron ,
mmm... Do you have any other suggestion? It's called a async because it the mutation to the state can be performed after the async completes.
There was a problem hiding this comment.
Yeah, that’s what confused me. It can be performed after the async completes. But in some cases it want. Actually it will last even longer (unbounded) if other threads keep queuing tasks. When you say that something is async, I would assume that it will just queue something and complete.
So I would name this something like queueOrExecuteAll (or something similar).
There was a problem hiding this comment.
Actually it will last even longer (unbounded) if other threads keep queuing tasks. When you say that something is async, I would assume that it will just queue something and complete.
Good point! I will rename it to queueOrExecuteAll. It communicates the intent better.
|
Btw. I suggest we drop the optional and introduce a breaking change. So that new versions actually have a form without the optional. In my app I use the Also, I don’t follow the second bullet ( One last question. Is |
I agree and in case anyone doesn't have time to do the migration he can still create a helper method.
It was removed in RxFeedback.swift.
I can do this in another pull request. In RxFeedback.swift, they renamed |
So How do you define a state machine? You have a |
|
@markotron |
|
Nice! I was hesitant a bit about that name :) Good! |
|
@eliekarouz — what are the reminding things here? Tests? |
| } | ||
| } | ||
|
|
||
| // queries | ||
| val State.loadNextPage: Optional<String> | ||
| val State.loadNextPage: String? |
There was a problem hiding this comment.
Now, on line 154 we pass null inside a chain because of this change:
// line 154
it.map { it.loadNextPage }.drive { showQuery(it) }
We had optional here because in rxJava you'll get NPE immediately if you pass null through the chain.
https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#nulls
This works here accidentaly because we're using Driver that has try-catch block inside it's map function so it swallows NPE but nulls are not supposed to be used in rxjava2.
That was also the reason why we used Optional for nextPageUrl and lastError (ok we currently don't map a state to these fields so that could remain nullable)
There was a problem hiding this comment.
@eliekarouz — @JurajBegovac is correct. Although you refactored feedback loops in a way that they don't require Optionals, this guy still needs it.
Instead of removing an optional here, you should convert it on line 162.
There was a problem hiding this comment.
@JurajBegovac , @markotron
Yeah you are right. I missed it in the map operator. Actually I didn't test the example yet.
There was a problem hiding this comment.
@JurajBegovac , @markotron , We can transform them to empty strings so we don't have to add the back the Optional. I know it's a bit dirty... A cleaner solution would be to add back the Optional but make it internal to the library or even public if you want.
| if (error is Optional.Some) { | ||
| Toast.makeText(this, error.data.displayMessage, Toast.LENGTH_SHORT).show() | ||
| private fun GithubPaginatedSearchActivity.showOrHideError(error: GitHubServiceError?) { | ||
| error?.let { |
There was a problem hiding this comment.
As I said in comment before - when we reach this place error would never be null because chain will throw NPE before (it cannot pass null) and Driver will swallow it (it looks like it works but it only works because of Driver)
Mainly tests but not sure if I will be able to do anything this weekend. |
This pull request includes the most general form of feedback loops that was implemented in RxFeedback.swift. TODO:
Unit Tests
This is still a draft as I still need to add the unit tests for the new feedback loops. I am not very familiar with unit testing with
TestScheduler. I will be doing some research to understand how it works.EquatableIn the Swift version of this library, we are forcing the
Queryto be equatable. There was an overload that takesareEqual: (Query, Query) -> Bool. This was removed from RxFeedback.swift . @kzaher I think this was needed for things like arrays because prior to Swift 4.2Array<Equatable>was notEquatable. but I think we still need this overload in Kotlin because we need to call special methods for deep comparison of arrays.If you agree, i will add an overload to the most general form that takes a
(Query, Query) -> Booleanparameter.Do you think we need this in Swift?
Optional<T>Regarding the
Optionaltype. I am not sure but I don't think this is needed. Maybe we can provide a new API without Optional and deprecate the current ones that use Optional. We can in a future version decide to remove them.However, we can keep the Optional type in the library as it can be used to wrap Kotlin nullables when we need to build streams with nullable values.