On 04/10/2021 17:51, Nir Lisker wrote:
I think that a PR can be created. The only point we need to decide is
about the subscription models we talked about above. ReactFX uses
something different for each, you use the same. That can determine if we
need different classes for each binding type.
We can introduce classes for these in any case if that keeps things more
clean; they're not public (after your change) so we can change our minds
later.
I'm not sure I see the difference though, ReactFX does the same kind of
observations to keep the bindings up to date. Here's a comparison:
OrElseConst (ReactFX) vs OrElseBinding
--------------------------------------
protected Subscription connect() {
return Val.observeInvalidations(src, obs -> invalidate());
}
------
protected Subscription observeInputs() {
return Subscription.subscribeInvalidations(source,
this::invalidate); // start observing source
}
OrElseGetBinding (no equivalent in ReactFX)
-------------------------------------------
@Override
protected Subscription observeInputs() {
return Subscription.subscribeInvalidations(source,
this::invalidate); // start observing source
}
FlatMapped (ReactFX) vs FlatMapBinding
--------------------------------------
protected final Subscription connect() {
return Val.observeInvalidations(src, obs -> srcInvalidated())
.and(this::stopObservingSelected);
}
private void stopObservingSelected() {
if(selectedSubscription != null) {
selectedSubscription.unsubscribe();
selectedSubscription = null;
}
}
------
protected Subscription observeInputs() {
Subscription subscription =
source.subscribeInvalidations(this::invalidate);
return () -> {
subscription.unsubscribe();
mappedSubscription.unsubscribe();
mappedSubscription = Subscription.EMPTY;
};
}
In both ReactFX and the PoC we see that FlatMapping requires a more
complicated subscription. As a FlatMap observes the original observable
and the result of the flat mapping, when unsubscribing two listeners
need to be unsubscribed.
As for the OrElse and OrElseGet, I just noticed ReactFX does not have an
#orElseGet equivalent. Closest thing is #getOrSupply but that doesn't
create a binding and is just a convenience method to get the current
value.
This does make me wonder how useful #orElseGet really would be -- I
think we should leave it out for now, it is easy to introduce later if
there is a real use case for it (I checked what I code I have, and I
never used it so far, it was only used to delegate #orElse to #orElseGet
which is pretty useless).
I think one more thing to decide is the naming of flatMap, there seems
to be some support for using "select" as a name instead. I personally
don't mind the name as for me flatMap just implies a map with the
additional step of flattening the result (stripping the extra wrapper,
be it Optional, Stream or in our case, ObservableValue).
I don't think that confusion with reactive programming is good argument
to not use this name, as neither Optional nor Stream have resulted in
such confusion.
I can create the JBS issue for this one and a draft CSR if you want.
It would be great to move this forward. If we can't borrow one of the
existing JBS issues then it would be good to create a new one, I can do
that, I can rewrite the text of the PoC MR a bit for this purpose. As
for a CSR, I'm not quite sure what that entails so it might be best if
you draft one.
The draft PR is mostly missing the JUnit test coverage now, which I can
take care off. I can also take care of the other changes we discussed
that may not be there yet. And I'll take another critical look at the
documentation and see if it still matches with what was discussed.
I've tried pushing the JUnit 5 upgrade as well, but I don't think it
will arrive in time (or perhaps it will). What I can do is convert my
JUnit 5 tests to JUnit 4, and once JUnit 5 is available I can revert that.
--John
