Most general form of feedback loops by eliekarouz · Pull Request #7 · NoTests/RxFeedback.kt

eliekarouz · 2019-05-05T14:44:10Z

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.
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 I think this was needed for things like arrays because prior to Swift 4.2 Array<Equatable> was not Equatable. 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) -> Boolean parameter.
Do you think we need this in Swift?
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.
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.

- 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

markotron · 2019-05-08T06:43:00Z

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

kzaher · 2019-05-08T18:33:11Z

@eliekarouz yes, that was because of compiler limitations :(.

kzaher · 2019-05-08T18:37:10Z

@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.

markotron · 2019-05-08T21:01:37Z

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

kzaher · 2019-05-09T12:20:30Z

observable state machine -> Observable
identifiable values (requests) -> Some value that has an identifier. The value is used to check out should new value be sent. Identifier is used to know where to send it.

eliekarouz · 2019-05-09T12:25:45Z

@markotron

Could you please
add some comments explaining what’s happening? For example, class comments
for the RequestLifetimeTracking and its nested classes.

Sure. Will work on it this weekend...

markotron

Thanks for the interesting PR! It was fun reviewing it :)

Also, does the AsyncSynchronized concept have a name? Is it used somewere else as well? I'd like to read more about it.

markotron · 2019-05-21T08:58:26Z

+                        queryLifetime.latestQuery.onNext(query)
+                    } else continue
+                } else {
+                    val subscription = CompositeDisposable() // SingleAssignmentDisposable


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.

Yeah you are right... I was not very convinced of what I was writing there...

Also, does the AsyncSynchronized concept 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...

@markotron , I think it's called AyncLock

markotron · 2019-05-21T09:23:23Z

+ * 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>,


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?

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...

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?

markotron · 2019-05-21T09:47:12Z

+
+    private val queue = mutableListOf<Mutate<State>>()
+
+    fun async(mutate: (State) -> Unit) {


Should this be named async? :)

@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.

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).

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.

markotron · 2019-05-21T22:05:11Z

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 arrow.Oprional and it’s annoying to have 2 optionals.

Also, I don’t follow the second bullet (equatable comment) in your PR message. In swift you don’t have the (Query, Query) -> Bool method?

One last question. Is Query the same as Request now? If yes, should we rename that?

eliekarouz · 2019-05-22T06:40:20Z

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 arrow.Oprional and it’s annoying to have 2 optionals.

I agree and in case anyone doesn't have time to do the migration he can still create a helper method.

Also, I don’t follow the second bullet (equatable comment) in your PR message. In swift you don’t have the (Query, Query) -> Bool method?

It was removed in RxFeedback.swift.

One last question. Is Query the same as Request now? If yes, should we rename that?

I can do this in another pull request. In RxFeedback.swift, they renamed Event to Mutation do you want me to do the same here too?

markotron · 2019-05-22T19:27:47Z

In RxFeedback.swift, they renamed Event to Mutation do you want me to do the same here too?

So Event --> Mutation and Query --> Request?

How do you define a state machine? You have a enum called State and an enum called Mutation (instead of Event)? What is the standard naming for mutations? Let's say that you have a login state machine and the user enters credentials, would you say that this emitted a CredentialsEntered mutation or CredentialsEntered event?

kzaher · 2019-05-24T13:55:23Z

@markotron
We've renamed Mutation -> Event again. That was the feedback we got.
Query -> Request was an improvement.

markotron · 2019-05-24T17:17:10Z

Nice! I was hesitant a bit about that name :) Good!

markotron · 2019-05-28T09:04:07Z

@eliekarouz — what are the reminding things here? Tests?

JurajBegovac · 2019-05-28T11:11:37Z


 // queries
-val State.loadNextPage: Optional<String>
+val State.loadNextPage: String?


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)

@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.

@JurajBegovac , @markotron
Yeah you are right. I missed it in the map operator. Actually I didn't test the example yet.

@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.

JurajBegovac · 2019-05-28T11:16:02Z

-    if (error is Optional.Some) {
-        Toast.makeText(this, error.data.displayMessage, Toast.LENGTH_SHORT).show()
+private fun GithubPaginatedSearchActivity.showOrHideError(error: GitHubServiceError?) {
+    error?.let {


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)

eliekarouz · 2019-05-28T12:24:40Z

@markotron

@eliekarouz — what are the reminding things here? Tests?

Mainly tests but not sure if I will be able to do anything this weekend.

eliekarouz added 4 commits May 5, 2019 13:47

Most general form of FLs:

f96b2fb

- Implemented initial version of AsyncSynchronized to synchronize state mutation.

- Implemented the most general form of feedback loops.

d96624d

- 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

Better handling of areEqual.

922f5e1

Added reactSafe for Equatable

6a53f2d

eliekarouz mentioned this pull request May 7, 2019

Non-equatable requests in 2.0 NoTests/RxFeedback.swift#53

Closed

Added documentation to internal classes.

4eee88c

markotron reviewed May 21, 2019

View reviewed changes

eliekarouz added 2 commits May 22, 2019 09:43

Removed CompositeDisposable.

1db0a31

Removed Optional from the library.

2026453

Rename async to enqueueOrExecuteAll

7224c72

JurajBegovac reviewed May 28, 2019

View reviewed changes


		private val queue = mutableListOf<Mutate<State>>()

		fun async(mutate: (State) -> Unit) {

Uh oh!

Conversation

eliekarouz commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markotron commented May 8, 2019 via email

Uh oh!

kzaher commented May 8, 2019

Uh oh!

kzaher commented May 8, 2019

Uh oh!

markotron commented May 8, 2019 via email

Uh oh!

kzaher commented May 9, 2019

Uh oh!

eliekarouz commented May 9, 2019

Uh oh!

markotron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markotron commented May 21, 2019

Uh oh!

eliekarouz commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markotron commented May 22, 2019

Uh oh!

kzaher commented May 24, 2019

Uh oh!

markotron commented May 24, 2019

Uh oh!

markotron commented May 28, 2019

Uh oh!

JurajBegovac May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliekarouz commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eliekarouz commented May 5, 2019 •

edited

Loading

eliekarouz commented May 22, 2019 •

edited

Loading

JurajBegovac May 28, 2019 •

edited

Loading