-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix for missing pre-binding events #156
base: master
Are you sure you want to change the base?
Conversation
9779093
to
e6f622c
Compare
with(Binding(connection, middleware)) { | ||
bindings.add(this) | ||
if (isActive) { | ||
accumulate() |
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.
What do you think of having accumulate return the subject so that it can be passed to subscribeWithLifecycle without needing a cast?
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.
Passing the subject to the subscribeWithLifecycle
method actually would break the solution. Apart from the above line, we call it from the bindConnections
function, which ensures that all the accumulations start before the bindings happen.
On the other hand, we could avoid the binding by adding type variables everywhere. I think it could significantly complicate the solution. WDYT about discussing it and maybe fixing it in another PR?
It'd be great to see some test scenarios for expectations covering bind-unbind-rebind cycles. |
e6f622c
to
2e17260
Compare
2e17260
to
583f54d
Compare
This PR resolves an issue related to the order of the bind operations that, in some cases, causes missing events due to the producer producing the events before the binding happens.
The idea is to accumulate the events until all the connections are bound and the subscriptions happen.
On the one hand, the
MviCore
library does not know about theObservable
until we call the bind method. A new component calledAccumulatorSubject
helps to accumulate the events produced by anObservable
until told to stop doing so anddrains
all the accumulated events. In this case, until all theConnections
are bound. On the other hand, a UnicastSubject inside theBinding
also collects the events once theConsumer
subscribes toObservable
.The reason behind adding
UnicastSubject
in the middle is that there might exist bounded but not subscribed connections. So if all the bindings are bound, we can drain them but keep the not-consumed events inUnicastSubjects
.Because the
UnicastSubject
accepts only a singleObserver
, and theBinder
class is reusable for the sameLifecycle
object (we can call thestart
andstop
operations multiple times), it creates a new instance for each binding every time we invoke thestart
method of theLifecycle
.Also, due to the usage of multiple instances of
Binder
for subscribing to differentLifecycle
events, we keep track of all theBinder
instances. We may use the sameObservables
as the source in distinctBinder
instances. So, once we bind all the connections from all theBinders
, we can drain theAccumulatorSubjects
.This is not the final solution, just an ideation around a possible one