FrangSierra/RxFirebase

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?