[WIP] Firestore platform interface (v2) #1633

ditman · 2019-12-13T04:42:07Z

Description

Continuation of the WIP for the cloud_firestore federation; platform interface + core plugin changes.

This v2 attempts to address the concerns raised in #1568.

This is still incomplete, but I've implemented the minimal scaffolding code so I can run the firestore core plugin tests on top of the new implementation. I'm using that test output to track progress, and ensure feature parity with the old version.

Missing things:

Actual MethodChannel implementations for most of the functionality; for now:

00:01 +9 -23: Firestore Query document pagination FieldPath assertions [E]                                                                                                              
  UnimplementedError: DocumentReferencePlatform::get() is not implemented
  package:cloud_firestore_platform_interface/src/interfaces/document_reference_platform.dart 59:5  DocumentReferencePlatform.get
  package:cloud_firestore/src/document_reference.dart 77:90                                        DocumentReference.get
  cloud_firestore_test.dart 1113:67                                                                main.<fn>.<fn>.<fn>

00:02 +23 -23: Some tests failed.
Tests in the cloud_firestore_platform_interface package.
Document all public members
Actual dartfmt and flutter analyze

Related Issues

[cloud_firestore] Add support for web flutter/flutter#45293

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

method channel implementations.

…w API.

…ports.

port tests to the new interface.

…riteBatch.

harryterkelsen

Initial comments as this is still WIP

harryterkelsen · 2019-12-13T18:16:06Z

..._firestore_platform_interface/lib/src/implementations/method_channel_document_reference.dart

+
+class MethodChannelDocumentReference extends DocumentReferencePlatform {
+  MethodChannelDocumentReference() {
+    channel.setMethodCallHandler(_callHandler);


it looks like you are calling setMethodCallHandler multiple times. I think this will cause the previous handler for that channel to be overwritten.

You're right, though each instance of the platform has its own copy of the MethodChannel (I HOPE)

@hterkelsen you were right, even across different instances of a MethodChannel, only the LAST call to setMethodCallHandler seems to be called.

@mklim said: "There's only one handler per channel. A way to thin the Handler code is to actually move the implementation of the methods into other classes, and write the Handler itself just to plumb from method call and response to the business logic and back."

harryterkelsen · 2019-12-13T18:17:54Z

..._firestore_platform_interface/lib/src/implementations/method_channel_document_reference.dart

+  }
+
+  @visibleForTesting
+  static const MethodChannel channel = MethodChannel(


in the original implementation I see that the channel is initialized with FirestoreMessageCodec. You probably need to move FirestoreMessageCodec to this package and make sure the channels are initialized with it.

Thanks, I think I didn't realize this. I'll get this done!

Oh my, this codec complicates things 😭

Because of how the Codec operates (it basically uses the frontend of the plugin to decode some data types), I changed the initialization code of the MethodChannel Implementations to allow to pass a codec in; that way we can pass a codec with access to "userland" from the plugin, without having to migrate EVERY data type defined in the plugin (I tried, it was painful, and still didn't solve my issue).

This is the problematic bit.

The code above probably explains why there's so much "static" code, and initialization guards in different places of the plugin; since we're creating new instances of the plugin all the time the message codec needs to hydrate a serialized DocumentReference!

Fixing this is very involved; we may be able to inject a factory function so the codec doesn't need to create instances in there, but maybe this particular part of the plugin could benefit from some extra design work (I didn't find any other place where layers are pierced like this, so far)

Is hydrating a serialized DocumentReference expensive? It's not clear to me how much overhead is involved in that, compared to the general overhead of moving serialized snapshots in and out of the Dart VM.

Currently, it is not very expensive because:

There's a bunch of init guards/static code that prevents multiple initialization

We hydrate the moment we see a DcoumentReference that needs to be hydrated.

However, logic-wise, it is expensive, because the deserialization needs to be able to basically instantiate a whole new plugin.

If we want to break that up, we'd probably need to hoist all the data types to the platform interface. At that moment, to maintain current feature parity, we'd need hydration on getters (or similar). The cost of that hydration would depend on the complexity of the returned data.

(Or maybe there's something I'm not seeing, there's a chance I've been focusing on this code for too long already :P)

harryterkelsen · 2019-12-13T18:19:46Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/types/write_batch.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+class PlatformWriteBatch {


docs. Also I think it would be clearer if this was named PlatformWriteBatchHandle

Yes, docs are missing pretty much everywhere. I'm giving myself some time before I migrate this to add *Handle at the end, but I agree it's clearer.

harryterkelsen · 2019-12-13T18:21:53Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/types/query.dart

+}
+
+// TODO: this needs to be more precise
+class PlatformQuerySnapshot {


You should probably model this class on https://firebase.google.com/docs/reference/android/com/google/firebase/firestore/QuerySnapshot

I would remove asMap() as well, and make the app-facing plugin use the typed data

Yeah, I've been avoiding this type for a while. I have a suspicion that the data that the backend sends back through the channel to the frontend is not entirely identical to the one documented, so I'll model this as I go (I might be wrong)

...loud_firestore/cloud_firestore_platform_interface/lib/src/interfaces/firestore_platform.dart

a custom MessageCodec, that comes from userland. Removed auto-init from the platform interfaces, so the plugin can pass the correct MessageCodec.

MethodChannelFirestore platform.

`PlatformWriteBatchHandle`

implementation.

ditman · 2019-12-14T02:42:02Z

Some more tests are passing now; after adding the Transaction implementation:

00:01 +27 -19: Some tests failed.

This way, all MethodChannel implementations can share the same MethodChannel instance, and register additional method handlers after the class has been created.

Also, simplify test initialization now that all MethodChannel implementations share the same instance.

ditman · 2019-12-16T23:58:04Z

Added Query implementation:

00:02 +36 -10: Some tests failed.

ditman · 2019-12-17T00:31:34Z

Added DocumentReference implementation.

00:01 +46: All tests passed!

ditman · 2019-12-17T20:26:38Z

(Currently fixing dartfmt and analyzer)

collinjackson · 2019-12-17T21:17:43Z

...es/cloud_firestore/cloud_firestore_platform_interface/lib/src/interfaces/query_platform.dart

+    _instance = instance;
+  }
+
+  // Actual interface


I am leaning towards removing this non-Dartdoc comment (in all PlatformInterface subclasses) since people will be follow the example of this plugin and it clutters up the class a bit. If the code is self-documenting only Dartdoc comments will be necessary.

collinjackson · 2019-12-17T21:21:48Z

...restore/cloud_firestore_platform_interface/lib/src/implementations/multi_method_channel.dart

+
+typedef _MultiMethodChannelHandler(MethodCall call);
+
+class MultiMethodChannel extends MethodChannel {


Wondering if we should move this into a package (in the Flutter SDK? in the plugin_platform_interface package?) since it will be useful for firebase_database and firebase_storage. /cc @amirh

collinjackson · 2019-12-17T21:23:16Z

packages/cloud_firestore/cloud_firestore/pubspec.yaml

@@ -15,6 +15,8 @@ flutter:
        pluginClass: FLTCloudFirestorePlugin

 dependencies:
+  cloud_firestore_platform_interface:
+    path: ../cloud_firestore_platform_interface


Should eventually be pointing to a hosted version (probably by splitting this into two PRs)

collinjackson · 2019-12-17T21:23:51Z

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

-      'transactionTimeout': timeout.inMilliseconds
-    });
+
+    // Wrap the user-supplied [TransactionHandler] into something that can be passed to the Platform implementation.


Suggested change

// Wrap the user-supplied [TransactionHandler] into something that can be passed to the Platform implementation.

// Wrap the user-supplied [TransactionHandler] into something that can be passed to the platform implementation.

collinjackson · 2019-12-17T21:31:01Z

packages/cloud_firestore/cloud_firestore/test/cloud_firestore_test.dart

+      };
+
+      // Ensure the MethodChannel platforms are initialized
+      Firestore.platform;


It feels unintuitive that this has side effects. What do you think about doing a method instead?

collinjackson · 2019-12-17T21:31:28Z

packages/cloud_firestore/cloud_firestore/test/cloud_firestore_test.dart

+      // Ensure the MethodChannel platforms are initialized
+      Firestore.platform;
+
+      // All the MethodChannel implementation share the same channel


Suggested change

// All the MethodChannel implementation share the same channel

// All the MethodChannel implementations share the same channel

collinjackson · 2019-12-17T21:32:58Z

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

+
+  /// The platform instance that talks to the native side of the plugin.
+  @visibleForTesting
+  static final FirestorePlatform platform = FirestorePlatform.instance ?? (FirestorePlatform.instance = MethodChannelFirestore(FirestoreMessageCodec()));


It seems a bit unintuitive to be doing the assignment like this.

ditman · 2019-12-20T02:46:38Z

(Closing in favor of #1670)

ditman added 11 commits December 12, 2019 16:05

[firestore_platform_interface] Add package scaffolding.

1fb5e9c

[firestore_platform_interface] Add first pass of split interfaces and

8b80635

method channel implementations.

[firestore_platform_interface] Add SetOptions type.

cd3c1c8

[firestore_platform_interface] Tighten types, reshuffle exports.

f91c6fb

[firestore] [WIP] Use new platform interface, port transactions to ne…

cc03bd7

…w API.

[firestore_platform_interface] Tighten types around Write Batches.

46f0759

[firestore] Migrate Write Batch to the platform interface.

795b287

[firestore_platform_interface] Some more types, and reshuffling of ex…

6421625

…ports.

[firestore] Finish porting the plugin to the platform interface, and

e4424d5

port tests to the new interface.

[firestore_platform_interface] Fix signature of callHandler methods.

4331bb0

[firestore_platform_interface] Add MethodChannel implementation for W…

2a711a4

…riteBatch.

ditman requested review from collinjackson, amirh and harryterkelsen December 13, 2019 04:42

googlebot added the cla: yes label Dec 13, 2019

harryterkelsen reviewed Dec 13, 2019

View reviewed changes

amirh reviewed Dec 13, 2019

View reviewed changes

...loud_firestore/cloud_firestore_platform_interface/lib/src/interfaces/firestore_platform.dart Outdated Show resolved Hide resolved

ditman added 8 commits December 13, 2019 12:35

[firestore_platform_interface] Make _tokens final, not const.

6a19d71

[firestore_platform_interface] MethodChannel implementations now require

27610a3

a custom MessageCodec, that comes from userland. Removed auto-init from the platform interfaces, so the plugin can pass the correct MessageCodec.

[firestore] Inject the FirestoreMessageCodec into the

55fa17a

MethodChannelFirestore platform.

[firestore_platform_interface] Cleanup imports

058ed75

[firestore_platform_interface] Rename PlatformWriteBatch to

732baed

`PlatformWriteBatchHandle`

[firestore] Rename PlatformWriteBatch to PlatformWriteBatchHandle

daccabc

[firestore_platform_interface] Add MethodChannelTransaction

c65a7dd

implementation.

[firestore] Remove superfluous cast.

5c5f713

ditman added 4 commits December 16, 2019 12:07

[firestore_platform_interface] Implement Query:getDocuments

843a36f

[firestore] Move Source to the platform interface.

5779f2d

[firestore_platform_interface] Add, and use MultiMethodChannel.

6c568dc

This way, all MethodChannel implementations can share the same MethodChannel instance, and register additional method handlers after the class has been created.

[firestore] Export Source from platform interface.

7967d41

Also, simplify test initialization now that all MethodChannel implementations share the same instance.

[firestore_platform_interface] Add implementation for DocumentReference.

831ee19

collinjackson mentioned this pull request Dec 17, 2019

[cloud_firestore] Added Flutter web support #1670

Closed

28 tasks

collinjackson reviewed Dec 17, 2019

View reviewed changes

ditman closed this Dec 20, 2019

ditman deleted the firestore_platform_interface branch January 14, 2020 01:45

firebase locked and limited conversation to collaborators Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Firestore platform interface (v2) #1633

[WIP] Firestore platform interface (v2) #1633

ditman commented Dec 13, 2019

harryterkelsen left a comment

harryterkelsen Dec 13, 2019

ditman Dec 13, 2019

ditman Dec 16, 2019

harryterkelsen Dec 13, 2019

ditman Dec 13, 2019

ditman Dec 13, 2019

ditman Dec 14, 2019

ditman Dec 16, 2019

collinjackson Dec 16, 2019 •

edited

Loading

ditman Dec 16, 2019

harryterkelsen Dec 13, 2019

ditman Dec 13, 2019

harryterkelsen Dec 13, 2019

ditman Dec 13, 2019

ditman commented Dec 14, 2019

ditman commented Dec 16, 2019 •

edited

Loading

ditman commented Dec 17, 2019

ditman commented Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

collinjackson Dec 17, 2019

ditman commented Dec 20, 2019


		typedef _MultiMethodChannelHandler(MethodCall call);

		class MultiMethodChannel extends MethodChannel {

	// Wrap the user-supplied [TransactionHandler] into something that can be passed to the Platform implementation.
	// Wrap the user-supplied [TransactionHandler] into something that can be passed to the platform implementation.

	// All the MethodChannel implementation share the same channel
	// All the MethodChannel implementations share the same channel

[WIP] Firestore platform interface (v2) #1633

[WIP] Firestore platform interface (v2) #1633

Conversation

ditman commented Dec 13, 2019

Description

Missing things:

Related Issues

Checklist

Breaking Change

harryterkelsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collinjackson Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Dec 14, 2019

ditman commented Dec 16, 2019 • edited Loading

ditman commented Dec 17, 2019

ditman commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Dec 20, 2019

collinjackson Dec 16, 2019 •

edited

Loading

ditman commented Dec 16, 2019 •

edited

Loading