Improved API support for Firebase Database. #89

collinjackson · 2017-05-31T23:33:47Z

Fixes flutter/flutter#10411
Fixes flutter/flutter#10421

eseidelGoogle · 2017-06-02T20:33:52Z

Some of @lukef and @KevinTheGray's firebase work is believed to be blocked on this. :) They're considering pushing off firebase work for a week if this needs more time to bake. Just checking in.

collinjackson · 2017-06-05T11:16:33Z

Most of the database code I've been working on has been Database UI improvements (fixes flutter/flutter#10439) and an improved example app. I'd like another day or so to test those more prior to landing.

Based on flutter/flutter#10411 and flutter/flutter#10421 it sounds like @lukef and@KevinTheGray mostly just need the core database additions. So I'm punting all the UI changes and example app into a separate PR and this PR is now just about adding the missing APIs (including tests).

I think this can land as soon as I get an LGTM on it.

goderbauer

LGTM

goderbauer · 2017-06-05T16:43:14Z

...abase/android/src/main/java/io/flutter/plugins/firebase/database/FirebaseDatabasePlugin.java

+    Map<String, Object> arguments = (Map<String, Object>) call.arguments;
+    switch (call.method) {
+      case "FirebaseDatabase#goOnline":
+        {


Does this need to be in a block?

It doesn't, I was trying to be consistent with the others but maybe it's better not to do a block. I've removed the braces for the methods that don't take arguments.

mravn-google

LGTM, mostly nits

mravn-google · 2017-06-05T19:45:06Z

...abase/android/src/main/java/io/flutter/plugins/firebase/database/FirebaseDatabasePlugin.java

-          reference.removeEventListener((ValueEventListener) observer);
-        } else {
-          reference.removeEventListener((ChildEventListener) observer);
+    Map<String, Object> arguments = (Map<String, Object>) call.arguments;


To avoid the unchecked cast warning, do

Map<String, Object> arguments = call.arguments();

mravn-google · 2017-06-05T19:46:41Z

...abase/android/src/main/java/io/flutter/plugins/firebase/database/FirebaseDatabasePlugin.java

+
+      case "FirebaseDatabase#setPersistenceEnabled":
+        {
+          boolean isEnabled = (boolean) arguments.get("enabled");


[nit] You can avoid cast with

Boolean isEnabled = arguments.get("enabled");

Similarly below.

mravn-google · 2017-06-05T20:00:52Z

packages/firebase_database/lib/src/database_reference.dart

+  /// priority (including no priority), they are sorted by key. Numeric keys
+  /// come first (sorted numerically), followed by the remaining keys (sorted
+  /// lexicographically).
+


Missing /// here?

mravn-google · 2017-06-05T20:02:23Z

packages/firebase_database/lib/src/database_reference.dart

+
+  /// Note that priorities are parsed and ordered as IEEE 754 double-precision
+  /// floating-point numbers. Keys are always stored as strings and are treated
+  /// as numbers only when they can be parsed as a 32-bit integer


[nit] Missing . at the end.

mravn-google · 2017-06-05T20:06:16Z

packages/firebase_database/lib/src/firebase_database.dart

+  /// operations when the network connection is restored.
+  ///
+  /// However by default your write operations and cached data are only stored
+  /// in-memory and will be lost when your app restarts. By setting this value


this value to YES => [enabled] to true

mravn-google · 2017-06-05T20:10:43Z

packages/firebase_database/lib/src/firebase_database.dart

+    return _channel.invokeMethod("FirebaseDatabase#goOnline");
+  }
+
+  /// Shuts down our connection to the Firebase Database backend until goOnline


[goOnline]

mravn-google · 2017-06-05T20:20:50Z

packages/firebase_database/lib/src/query.dart

+  /// than or equal to the given key.
+  Query endAt(dynamic value, { String key }) {
+    assert(!_parameters.containsKey('endAt'));
+    return _copyWithParameters({ 'endAt': value, 'endAtKey': key});


[nit] Missing space before }

mravn-google · 2017-06-05T20:22:05Z

packages/firebase_database/lib/src/query.dart

+
+  /// Generate a view of the data sorted by values of a particular child key.
+  ///
+  /// Intended to be used in combination with startAt(), endAt(), or equalTo().


[startAt()], [endAt()], [equalTo()]

More places below

mravn-google · 2017-06-05T20:29:13Z

packages/firebase_database/test/firebase_database_test.dart

+  });
+}
+
+class MockPlatformChannel extends Mock implements MethodChannel { }


Remove this?
And maybe end the file with a new-line.

mravn-google · 2017-06-05T20:30:15Z

packages/firebase_database/test/firebase_database_test.dart

+
+import 'dart:async';
+
+import 'package:mockito/mockito.dart';


Are you using mockito for anything? Seems your mock channel isn't used.

collinjackson · 2017-06-05T22:38:12Z

Thanks for the detailed feedback! I'll make these changes in a subsequent PR.

collinjackson · 2017-06-20T08:06:02Z

Remaining code review feedback implemented in #144

Improved API support for Firebase Database for 0.0.6 * Various APIs added to FirebaseDatabase and Query * Added removal and priority to DatabaseReference * Improved documentation * Added unit tests

googlebot added the cla: yes label May 31, 2017

collinjackson changed the title ~~Improved API support for Firebase Database.~~ WIP: Improved API support for Firebase Database. May 31, 2017

collinjackson mentioned this pull request Jun 1, 2017

Firebase Database Lib fails with onChildChanged flutter/flutter#10439

Closed

collinjackson force-pushed the more_database branch from a1c3a9a to bd43dbc Compare June 5, 2017 09:40

collinjackson changed the title ~~WIP: Improved API support for Firebase Database.~~ Improved API support for Firebase Database. Jun 5, 2017

collinjackson force-pushed the more_database branch from bd43dbc to d6e204f Compare June 5, 2017 11:11

Improved API support for Firebase Database.

697c4d8

collinjackson force-pushed the more_database branch from d6e204f to 697c4d8 Compare June 5, 2017 11:19

collinjackson requested review from goderbauer and mravn-google June 5, 2017 11:22

goderbauer approved these changes Jun 5, 2017

View reviewed changes

Review feedback

dcb1a18

collinjackson force-pushed the more_database branch from 1f5fad8 to dcb1a18 Compare June 5, 2017 17:38

collinjackson merged commit b7040ef into flutter:master Jun 5, 2017

collinjackson deleted the more_database branch June 5, 2017 19:29

mravn-google reviewed Jun 5, 2017

View reviewed changes

collinjackson mentioned this pull request Jun 20, 2017

Firebase database limitToLast not working on Android flutter/flutter#10823

Closed


		import 'dart:async';

		import 'package:mockito/mockito.dart';

Improved API support for Firebase Database. #89

Improved API support for Firebase Database. #89

Uh oh!

Conversation

collinjackson commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eseidelGoogle commented Jun 2, 2017

Uh oh!

collinjackson commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mravn-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson commented Jun 5, 2017

Uh oh!

collinjackson commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

collinjackson commented May 31, 2017 •

edited

Loading

collinjackson commented Jun 5, 2017 •

edited

Loading