Changed behaviour for RxFirebaseDatabase.observeValueEvent
gswierczynski opened this issue · 6 comments
Hi,
Last version added check datasnapshot.exists() in the above method. As long as adding that in observeSingleValueEvent makes sense (probably everyone using lib were filtering on it anyway), adding this in observeValueEvent changes behaviour significantly.
There is no way to know now if the passed DatabaseReference exists or not (to act accordingly).
IMO it should be left to the lib user to deal with that (maybe warn in java doc).
Cheers,
Greg.
Hi @gswierczynski ,
you can know if the dataSnapshot exist or not just listening the onComplete
callback, if the DataSnapshot
doesn't exist, onComplete
will be call, but onSucess
will be not.
Hi @FrangSierra,
Thanks for the good work.
I don't think onComplete will be called for the observeValueEvent.
return Flowable.create(new FlowableOnSubscribe<DataSnapshot>() {
@Override
public void subscribe(final FlowableEmitter<DataSnapshot> emitter) throws Exception {
final ValueEventListener valueEventListener = new ValueEventListener() {
@Override
public void onDataChange(DataSnapshot dataSnapshot) {
if (dataSnapshot.exists())
emitter.onNext(dataSnapshot);
}
@Override
public void onCancelled(final DatabaseError error) {
emitter.onError(new RxFirebaseDataException(error));
}
};
emitter.setCancellable(new Cancellable() {
@Override
public void cancel() throws Exception {
query.removeEventListener(valueEventListener);
}
});
query.addValueEventListener(valueEventListener);
}
}, strategy);
EDIT:
And for Flowable I do not think it would be a good behaviour. As we would not be able to listen on a database reference that does not exists yet.
You are totally right, I missunderstood it with observeSingleValueEvent
which uses an onComplete
call too.
I will look how to resolve it, cause if I delete the dataSnapshot.exists()
the GenericTypedDataSnapshotMapper
will throw NPE in that case.
If you have any idea, please, feel free to do a PR and we can look together for a solution.
Have a good day.
Please see #29 for my take on this.
Maybe wrap Datasnapshot in optional?