firebase_database support for onDisconnect#578
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
cc @collinjackson for review |
|
Hi I would like to see this upstream. Is there anything I can do? |
|
@rmarau The cla issue appears to be that two accounts were used to write the commits, @demarcoHome and @rmarau. We need both to say that they are ok with submitting their code in this PR. The actual PR itself then needs to be reviewed, which can be done by @collinjackson, @kroikie, or @bparrishMines. Looking at the code quickly myself my first guess would be that we'll need to add tests before it can be submitted. |
|
Thanks for the update. I'm ok with the use of these commits as a contribution to this project (flutter-plugins). |
| }]; | ||
| } else if ([@"OnDisconnect#set" isEqualToString:call.method]) { | ||
| [getReference(database, call.arguments) onDisconnectSetValue:call.arguments[@"value"] | ||
| andPriority:call.arguments[@"priority"] |
There was a problem hiding this comment.
Since priority could not exist here we should check for it and use the appropriate setValue method.
There was a problem hiding this comment.
My experience in Obj-C is not that great. Will need help on this.
I applied the same pattern as for DatabaseReference#set where priority is also optional.
There was a problem hiding this comment.
I see, @rmarau you are correct. In a future PR we should update both this and DatabaseReference#set to check for priority since having null priority removes previously set priorities.
| if (priority != null) { | ||
| reference | ||
| .onDisconnect() | ||
| .setValue(value, (double) priority, new DefaultCompletionListener(result)); |
There was a problem hiding this comment.
Since this can be either a String or a double we should not cast this to double without checking.
There was a problem hiding this comment.
Correct.
At some point I tried to take a similar approach as in DataReference.setValue.
Priority argument comes as dynamic from dart and mapped as Object to java.
Now the difference is in the android API: OnDisconnect.setValue holds 3 signatures for the priority argument, being: double, Map and String, whereas DataReference.setValue is more generic and exposes a single signature with Object. Will update to
if (priority instanceof String) {
reference.onDisconnect()
.setValue(value, (String) priority, new DefaultCompletionListener(result));
} else if (priority instanceof Double) {
reference.onDisconnect()
.setValue(value, (double) priority, new DefaultCompletionListener(result));
} else if (priority instanceof Map) {
reference.onDisconnect()
.setValue(value, (Map) priority, new DefaultCompletionListener(result));
}
|
@rmarau Could you also add some tests for these new methods? |
|
Added tests.
Tried to rebase the code and the PR is now a mess. |
|
CLAs look good, thanks! |
|
@rmarau I forced existing changes to new commit. You should be able to continue work on this PR now. Thanks for your help. |
|
@rmarau Will you have the opportunity to integrate the above review comments? It's totally fine if you don't have time to do this, we'd be happy to (eventually) take care of it. Thanks for your contribution! |
|
@Hixie I did integrate the comments. The PR has a single commit because I messed with a master rebase and kroikie helped by cleaning up to a single commit.
As for the comment on the IOS implementation, I explained that I dont have the skills/time for that. You are free to take it from here. |
Adds support for OnDisconnect mimicking Android's api.
FirebaseDatabase.instance.reference().child("path").onDisconnect().set(1).then ...
FirebaseDatabase.instance.reference().child("path").onDisconnect().cancel().then ...
FirebaseDatabase.instance.reference().child("path").onDisconnect().remove().then ...
FirebaseDatabase.instance.reference().child("path").onDisconnect().update().then ...
Tested ok with android and ios.