[WIP] Federated cloud firestore platform interface by ditman · Pull Request #1568 · firebase/flutterfire

ditman · 2019-12-06T01:43:31Z

Description

WIP the platform interface + native plugin changes for cloud_firestore federation.

Many things missing, chief of which are:

Finish the actual code:
- Migration of the Document Reference Snapshots to a Stream + map on the plugin.
- Add types for some interesting return values (like Queries, and Document References)
Verify that the plugin still works as intended on the emu.
Many tests

ditman · 2019-12-06T01:50:47Z

(Force-pushed after rebasing with the latest master)

…napshots' This is to prevent naming collisions with the same method from Document References.

collinjackson

I have one high level concern about how we're structuring the platform interface (see below) and some style nits that apply throughout this PR:

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-a-trailing-comma-for-arguments-parameters-and-list-items-but-only-if-they-each-have-their-own-line

Use a trailing comma for arguments, parameters, and list items, but only if they each have their own line.

Example:

List<int> myList = [
  1,
  2,
];
myList = <int>[3, 4];

foo1(
  bar,
  baz,
);
foo2(bar, baz);

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods

Only use => when everything, including the function declaration, fits on a single line.

collinjackson · 2019-12-06T01:54:39Z

packages/cloud_firestore/cloud_firestore/example/.flutter-plugins-dependencies

@@ -0,0 +1 @@
+{"_info":"// This is a generated file; do not edit or check into version control.","dependencyGraph":[{"name":"cloud_firestore","dependencies":["firebase_core"]},{"name":"firebase_core","dependencies":[]}]}


Should not be checked in to version control

I have no idea where this is coming from; I'll remove it from the PR, and add it to .gitignore

packages/cloud_firestore/cloud_firestore/lib/src/document_reference.dart

collinjackson · 2019-12-06T01:57:01Z

packages/cloud_firestore/cloud_firestore/lib/src/firestore.dart

@@ -10,28 +10,6 @@ part of cloud_firestore;
 class Firestore {
  Firestore({FirebaseApp app}) : app = app ?? FirebaseApp.instance {
    if (_initialized) return;


This can be removed now.

...oud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_cloud_firestore.dart

collinjackson · 2019-12-06T02:39:00Z

...oud_firestore/cloud_firestore_platform_interface/test/method_channel_firebase_core_test.dart

+  TestWidgetsFlutterBinding.ensureInitialized();
+
+  group('$MethodChannelCloudFirestore', () {
+    final MethodChannelCloudFirestore channelPlatform =


Lots more to write here :)

Indeed 😭

…s stream. (Corrected return type of Stream controller for the Query snapshots as well)

collinjackson · 2019-12-07T00:22:24Z

...oud_firestore/cloud_firestore_platform_interface/lib/cloud_firestore_platform_interface.dart

+  }
+
+  // Document Reference
+  Future<void> setDocumentReferenceData(


@amirh Does this seem right to you as a style convention for federated plugins that are wrapping instances of native classes other than the main class of the plugin?

Right now we're shoehorning everything onto CloudFirestorePlatform instead of having a PlatformDocumentReference that has the same extends-only semantics as CloudFirestorePlatform.

If we were ok with being less idiomatically Dart we could call it something like DocumentReference_setData... not sure that's actually better but I just want to make sure that we're ok with the pattern being established here.

I think it's definitely makes sense for the platform interface to be composed from multiple classes. In fact we've already done this in the webview plugin (WebViewPlatform and WebViewPlatformController). I think it's less of a convention thing and more of a "whatever design works fits better for a given plugin".

For this case, after discussing with Collin, it seems like multiple classes is a better choice.

…l package)

collinjackson

Per @amirh's comment, let's split PlatformDocumentReference, PlatformWriteBatch, etc. into their own classes.

ditman · 2019-12-09T23:24:52Z

Per @amirh's comment, let's split PlatformDocumentReference, PlatformWriteBatch, etc. into their own classes.

Yep, I'll try to get this refactored.

andrea689 · 2019-12-10T19:22:55Z

...oud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_cloud_firestore.dart

+
+  void _handleDocumentSnapshot(MethodCall call) {
+    final int handle = call.arguments['handle'];
+    _queryObservers[handle].add(call.arguments);


_queryObservers should be _documentObservers, right?

Yep, good catch!

andrea689 · 2019-12-10T19:24:11Z

...oud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_cloud_firestore.dart

+    );
+  }
+
+  static final Map<int, StreamController<int>> _documentObservers =


Map should be Map<int, StreamController<dynamic>>

andrea689 · 2019-12-10T19:24:31Z

...oud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_cloud_firestore.dart

+    );
+  }
+
+  static final Map<int, StreamController<int>> _queryObservers =


Map should be Map<int, StreamController<dynamic>>

ditman · 2019-12-11T00:59:07Z

(I'm going to close this, so no more effort is wasted here. Apologies @andrea689, I'll keep your reviews in mind in my next iteration of this! And thanks again!)

ditman requested a review from harryterkelsen December 6, 2019 01:43

ditman requested review from collinjackson and kroikie as code owners December 6, 2019 01:43

googlebot added the cla: yes label Dec 6, 2019

ditman added 6 commits December 5, 2019 17:46

Package scaffolding and initial docs.

51101c9

cloud_firestore_platform_interface [wip]

bbf60ed

[cloud_firestore] Use the platform interface

4254aef

[cloud_firestore_platform_interface] Add transaction delegation.

4487365

[cloud_firestore] Migrate runTransaction to platform interface.

0f505c5

[cloud_firestore_platform_interface] Move query snapshots to a stream.

fe72775

ditman removed request for collinjackson and kroikie December 6, 2019 01:51

[cloud_firestore] Use the stream from the plugin.

3f124b9

collinjackson self-requested a review December 6, 2019 01:54

ditman added 2 commits December 5, 2019 18:16

[cloud_firestore_platform_interface] Rename 'snapshots' to 'getQueryS…

85a4073

…napshots' This is to prevent naming collisions with the same method from Document References.

[cloud_firestore] Use renamed getQuerySnapshots

0cfd3d7

collinjackson reviewed Dec 6, 2019

View reviewed changes

ditman added 5 commits December 5, 2019 19:03

[cloud_firestore_platform_interface] Create DocumentReferenceSnapshot…

32fa7ee

…s stream. (Corrected return type of Stream controller for the Query snapshots as well)

[cloud_firestore] Use the new stream for document_references.

d0663ce

[cloud_firestore] dartfmt

6fd3a00

[cloud_firestore_platform_interface] dartfmt

6610d18

[cloud_firestore_platform_interface] Address PR feedback

69906ee

collinjackson reviewed Dec 7, 2019

View reviewed changes

ditman added 2 commits December 6, 2019 19:44

[cloud_firestore] Some PR nits

75ab8e9

[cloud_firestore_platform_interface] [wip] Adding tests (from origina…

5bf190e

…l package)

collinjackson self-requested a review December 9, 2019 21:16

collinjackson reviewed Dec 9, 2019

View reviewed changes

andrea689 suggested changes Dec 10, 2019

View reviewed changes

ditman closed this Dec 11, 2019

ditman mentioned this pull request Dec 13, 2019

[WIP] Firestore platform interface (v2) #1633

Closed

13 tasks

firebase locked and limited conversation to collaborators Aug 6, 2020

		@@ -0,0 +1 @@
		{"_info":"// This is a generated file; do not edit or check into version control.","dependencyGraph":[{"name":"cloud_firestore","dependencies":["firebase_core"]},{"name":"firebase_core","dependencies":[]}]} No newline at end of file

Conversation

ditman commented Dec 6, 2019

Description

Uh oh!

ditman commented Dec 6, 2019

Uh oh!

collinjackson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson left a comment

Choose a reason for hiding this comment

Uh oh!

ditman commented Dec 9, 2019

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!

ditman commented Dec 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

collinjackson left a comment •

edited

Loading

collinjackson Dec 7, 2019 •

edited

Loading