Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Fixes #134 and #28 #146

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes #134 and #28 #146

wants to merge 3 commits into from

Conversation

jwiesmann
Copy link

Using keyRef.updateChildren instead of keyRef.setValue also fixes a huge amount of unnecessary 'onDataExited(DataSnapshot dataSnapshot)' events, which has not been reported yet. keyRef.updateChildren understands if the data needs to be inserted or updated. No need to use setValue here. Since priorities are not relevant anymore (https://stackoverflow.com/questions/31577915/what-does-priority-mean-in-firebase) i decided to not consider them anymore.

…ad of keyRef.setValue also fixes a huge amount of unnecessary 'onDataExited(DataSnapshot dataSnapshot)' events which has not been reported yet. keyRef.updateChildren understands if the data needs to be inserted or updated. No need to use setValue here. Since priorities are not relevant anymore (https://stackoverflow.com/questions/31577915/what-does-priority-mean-in-firebase) i decided to not consider them anymore.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@jwiesmann
Copy link
Author

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@jwiesmann
Copy link
Author

lets try again :D

@jwiesmann
Copy link
Author

..

@jwiesmann jwiesmann closed this Sep 1, 2018
@jwiesmann jwiesmann reopened this Sep 1, 2018
@@ -212,7 +215,7 @@ public void onComplete(DatabaseError databaseError, DatabaseReference databaseRe
}
});
} else {
keyRef.setValue(null);
keyRef.setValueAsync(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain your decision to move to Async here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like, that the firebase developer changed the name of that function if you do not add a CompletionListener. Before you could call setValue without listener, now you have to call setValueAsync if you do not want to use a listener. Since there is no listener required, i changed it to setValueAsync.

Copy link
Author

@jwiesmann jwiesmann Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to mention. I assume this is the fix for #28, since this function has been changed! (if you check some posts on stackoverflow.com, you will see that they suggest you to use a CompletionListener, which would call the 'old' function) And SetValue must have always been async

@samtstern
Copy link
Contributor

@jwiesmann thanks for sending this! The core (using update) LGTM, I just want to understand your reasoning on some minor points.

@samtstern
Copy link
Contributor

Also don't worry about the CLA, I manually checked and you're good to go:
image

@samtstern
Copy link
Contributor

samtstern commented Sep 4, 2018

@hiranya911 would love your input here.

It seems like in the past, geofire was relying on the coincidence of the Android and Java RTDB SDKs having the same API surface. Then we use provided dependencies to provide the right code at runtime.

The recent Java changes break this (which is fine) but I wonder how we should proceed here. Should we lock geofire into the 5.x version of the Admin SDK forever?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants