-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I have some minor feedback
[batch deleteDocument:reference]; | ||
result(nil); | ||
} else if ([@"Batch#commit" isEqualToString:call.method]) { | ||
// TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to leave this empty TODO?
I think you should remove the batches from the NSDictionary once they are committed
WriteBatch batch = batches.get(handle); | ||
Task<Void> task = batch.commit(); | ||
addDefaultListeners("commit", task, result); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should remove the batches from the SparseArray once they are committed.
final List<Future<Null>> _actions = <Future<Null>>[]; | ||
|
||
/// Indicator to whether or not this [WriteBatch] has been committed. | ||
bool committed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a private property to follow the convention of the Firebase SDK
@@ -36,9 +37,11 @@ | |||
|
|||
// Handles are ints used as indexes into the sparse array of active observers | |||
private int nextHandle = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please rename this nextListenerHandle to better distinguish it from the property below?
Thanks for the feedback. Good catch on removing from the Arrays. That was a memory leak waiting to happen. Can you confirm for me that it works as expected on iOS? I'm just going by example/instinct with the ObjC code. Edit: Travis definitely doesn't like it, so that's not a good sign. |
Apologies for the messy ObjC. Thanks for the cleanup, @collinjackson |
1b7f1c2
to
6c1ad58
Compare
After taking a second look at this, I think we want batches to be produced by an instance method of |
Love the design so far. Will anything need to change on the native side to make that possible? Presently we're using |
When we implement multiple app support we can fix the native side to look up the appropriate Firestore instance. The API on the Dart side won't change so we won't need to worry about breaking anyone. |
Any idea for when this will be released? |
Looking for this too.. |
For uniformity with Android/iOS classes Rename file parts
614e80e
to
70de28e
Compare
is there any docs for this? :) |
Fixes flutter/flutter#14026
I've done some initial testing on Android, including a sync call to an operation followed by an async call.
iOS support is here but needs to be tested.