[cloud_firestore] Created firestore platform interface #1686

amrfarid140 · 2019-12-20T15:38:06Z

Description

This PR is the first of a set of 3 PRs to enable Flutter Web support for Cloud Firestore.

All the discussion around approach and logic are being actively discussed here.

Related Issues

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.

amrfarid140 · 2019-12-20T15:39:06Z

@ditman That's the first one to get cloud_firestore_platform_interface published.

ditman · 2019-12-21T01:18:22Z

@collinjackson, this is the real deal. Most of the work has happened here, but this is what we want to land. PTAL when you have a chance!

ditman · 2019-12-21T01:32:27Z

(I'll also be taking another look)

ditman

I think this looks great, there's a bunch of double quotes across the code, but those can be added later, alongside tightening the lint rules. Only one general comment, the structure of the package under src is a little bit too flat, I think it'd be cool to have a few directories to separate interfaces from implementations (and maybe types). Since everything is part'd in, it should be easy to add afterwards.

Let's wait for Collin's review, but other than the minor nitpicks in this review, I can't find anything that is immediately problematic.

Again, thanks for all this code!

packages/cloud_firestore/cloud_firestore_platform_interface/analysis_options.yaml

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

...oud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_document_change.dart

...ud_firestore_platform_interface/lib/src/platform_interface/document_reference_interface.dart

packages/cloud_firestore/cloud_firestore_platform_interface/test/field_value_test.dart

...ages/cloud_firestore/cloud_firestore_platform_interface/lib/src/firestore_message_codec.dart

ditman

88           ,ad8888ba,  888888888888  88b           d88  
88          d8"'    `"8b      88       888b         d888  
88         d8'                88       88`8b       d8'88  
88         88                 88       88 `8b     d8' 88  
88         88      88888      88       88  `8b   d8'  88  
88         Y8,        88      88       88   `8b d8'   88  
88          Y8a.    .a88      88       88    `888'    88  
88888888888  `"Y88888P"       88       88     `8'     88

Looks Good To Me

Analyzer is complaining about this extra carriage return.

collinjackson

Thanks so much for contributing this change! A web implementation of Firestore in FlutterFire is a top requested feature and I'm sure this will help a lot of people.

Here's my initial round of comments; I'm expect I may have a few more once these are addressed.

Please make sure to follow the Effective Dart guide for Dart throughout. In particular please follow these guidelines:

have a blank line after the first sentence
put a period at the end of the first line
avoid redundancy with the surrounding context (e.g. no need to repeat the class name in the constructor's documentation)

Thanks!!

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

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/document_snapshot.dart

...loud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_query_snapshot.dart

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

collinjackson · 2020-01-07T00:42:20Z

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

+    throw UnimplementedError("withApp() not implemented");
+  }
+
+  /// Firebase app name


Would it make sense to expose a FirebaseApp here instead?

FirebaseApp is already exposed to avoid breaking changes. This is just a convenience used in few places in here

collinjackson · 2020-01-07T00:50:16Z

...ges/cloud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_firestore.dart

+  static int _transactionHandlerId = 0;
+
+  @override
+  FirestorePlatform withApp(FirebaseApp app) =>


Wondering if we can call this instanceFor({ FirebaseApp app }) to match #1377

At this point we already have a factory method of the FirestorePlatform called instanceFor. We won't be able to rename this as well to instanceFor. Also this is only made public so FirestoreWeb (in the web plugin) is able to override it and provide a web-specific implementation.

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

grundid · 2020-01-07T17:46:32Z

A small suggestion here: can we have a flutter independent API for firestore? As this package mainly consists of interfaces it would be great to move them into a new package that is pure dart and then depend on it.

There are already many attempts to create platform independent APIs for firestore. We ourself also created one (https://github.com/dartclub/firestore_dart).
Based on our API we are working on a firestore serializer (similar to the json_serializer) that is able to generate code to read and write data to and from firestore. To make this serializer universal we depend on an API for firestore.

ditman · 2020-01-07T21:36:06Z

can we have a flutter independent API for firestore? As this package mainly consists of interfaces it would be great to move them into a new package that is pure dart and then depend on it.

@grundid that's a great suggestion, but for now we're focusing on merging this, so people can use Firestore on their Flutter apps. Further abstraction sounds possible (but it also sounds as a v2 project for this change)

Co-Authored-By: Collin Jackson <[email protected]>

…' into firestore_web/platform_interface

Co-Authored-By: Collin Jackson <[email protected]>

…' into firestore_web/platform_interface

ditman · 2020-01-17T21:33:08Z

@amrfarid140 I was just finishing testing this on my new test app. Great stuff!

ditman

I think we should ship this. What do you think @collinjackson?

amrfarid140 · 2020-01-28T17:37:51Z

@collinjackson any updates or feedback on this PR? Really want to merge to move on with other parts of the web implementation as well.

ditman · 2020-01-29T20:26:16Z

Hey @amrfarid140! Apologies for the radio silence!

We're about to send you a PR with a few little changes to the platform interface (and things that depend on them). Most of them are some moving around of files (so files regarding platform interfaces live together, same with files regarding the method channel), and some renaming so everything is consistent.

The only (big-ish) issue that was found was that the Web versions of the platform interfaces are using implements, and that basically means that we cannot change the interface, or it'll break the web implementation.

I'm currently adding the extends PlatformInterface to all the PlatformInterface objects, so we can prevent the implements from happening, and it is intentional.

Bear in mind that this PR is going to be used by many as an "example" on how to build their own plugins, so we need to make sure there's nothing very dangerous here (at least, that we know of now).

So by EOD today I'll cut a PR to your repo with the changes to the platform interface and method channel code, and maybe some changes to the Web implementation (just to keep the branch functional!)

I'll get back to you ASAP. If you want to privately message me, feel free to do so at: (first half of my github username) at google dot com ;)

amrfarid140 · 2020-01-29T20:47:44Z

Thanks for this update, I am happy that things are moving along. I will keep an eye for your PR and aim to get it merge and everything update ASAP.

…

On Wed, 29 Jan 2020, 8:26 pm David Iglesias, ***@***.***> wrote: Hey @amrfarid140 <https://github.com/amrfarid140>! Apologies for the radio silence! We're about to send you a PR with a few little changes to the platform interface (and things that depend on them). Most of them are some moving around of files (so files regarding platform interfaces live together, same with files regarding the method channel), and some renaming so everything is consistent. The only (big-ish) issue that was found was that the Web versions of the platform interfaces are using implements, and that basically means that we cannot change the interface, or it'll break the web implementation. I'm currently adding the extends PlatformInterface to all the PlatformInterface objects, so we can prevent the implements from happening, and it is intentional. Bear in mind that this PR is going to be used by many as an "example" on how to build their own plugins, so we need to make sure there's nothing very dangerous here (at least, that we know of now). So by EOD today I'll cut a PR to your repo with the changes to the platform interface and method channel code, and maybe some changes to the Web implementation (just to keep the branch functional!) I'll get back to you ASAP. If you want to privately message me, feel free to do so at: (first half of my github username) at google dot com ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1686>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGFK4NNX7E24JZIC5IYJKLRAHQ6VANCNFSM4J57OA7Q> .

ditman · 2020-02-03T22:23:37Z

Hey @amrfarid140, it seems I can't create a pull request on your fork. Do you mind giving me permissions to do that? Thanks!

ditman · 2020-02-03T22:42:28Z

Hey @amrfarid140, I still can't create PRs on your fork (maybe some weird github issue?) but I've pushed my changes here:

https://github.com/amrfarid140/flutterfire/commits/amr-master-pr

This is prepared on top of your master branch. We've touched all packages, and not just the platform interface (so future PRs for the other 2 packages should be smoother as well).

I think @collinjackson still wants to propose some changes, but I can help with those pretty quickly, once the PR(s) are updated with the above.

The bulk of the changes was to remove the API surface exposed to the world through the different packages of the plugin (MethodChannel implementation, Web implementation), and ensure the Platform classes all extended PlatformInterface so they are extended vs implemented.

I'd say by "size":

changes are moving files around,
the extends thing and
making things that shouldn't be imported by the end user "private".
Some formatting and dart analysis and all that.

I ran tests and they seem to pass.

amrfarid140 · 2020-02-03T22:49:03Z

Yeah that's weird @ditman , I've added you as a collaborator and made sure that "allow edits from maintainers" is checked. Anyways, Thanks so much for those changes. I will get all the packages sorted and all those changes to the master branch to update the PRs ASAP.

I will also do some testing as well to ensure that we are all good.

Thanks @ditman

ditman · 2020-02-04T03:49:04Z

@amrfarid140 @collinjackson and I've been working on some extra changes to the branch throughout the day. There's one big change that we want to get in, though, which should simplify the FieldValues a little bit.

It is late here, but I'll get some stuff done tomorrow (I can prepare another branch/PR if you don't want to wait for that change)

amrfarid140 · 2020-02-04T07:14:03Z

@ditman Maybe it makes sense then to wait for you guys to finish all the changes before merging them to master

ditman · 2020-02-05T03:12:30Z

@amrfarid140 I think we're done! Take a look at the changes!

I've tested on my end and it seems to be working; but the more we test, the merrier.

Thanks for your patience!

collinjackson · 2020-02-05T03:31:23Z

@amrfarid140 Thank you SO much for the time you've put into this patch. I've been pairing with @ditman on this, and what our changes are doing is reducing fragility -- hiding classes and methods from the app-facing package if they're only useful to platform implementers, minimizing the platform interface surface area (making MethodChannel-related classes private), and ensuring that the platform interface classes that we think might change in the future extend PlatformInterface and use verifyExtends so that we can make changes without breaking platform implementers.

A few changes of note:

All *Platform classes are now hidden from the app-facing API.
FieldValue uses a static method on the platform class FieldValuePlatform to keep the exposed app-facing API minimal.
We don't think that it should be necessary to decode FieldValues except in unit tests so that code has been removed from the package. Hopefully this won't be a breaking change.
Web version now delegates to the native auto id generator.
Removed a confusing use of @visibleForTesting, relying on import visibility instead
Removed appName() in favor of app.name

Let us know if there's anything that seems off or confusing! I'm excited to see this landed.

amrfarid140 · 2020-02-05T09:06:56Z

@ditman @collinjackson Thanks so much for improving this PR. I've gone through the changes and all makes sense to me.

Everything is merged now to amrfarid140/master branch and both PRs have been updated.

amrfarid140 · 2020-02-05T09:40:35Z

Tests are failing on create_simulator.. we need to re-run them again and see what happens.

collinjackson

You're working off an outdated version of the FlutterFire repo. You'll need to merge the upstream master before we can land this. Besides changelog there was one minor conflict (you had a different solution than @kroikie for #1859 and I think we should go with his version). I resolved it in a way that made sense to me; here is a branch showing that: https://github.com/collinjackson/flutterfire/tree/amr-master-merge

Merging this commit will resolve the iOS simulator failures: 16ed623#diff-0f7067c31caedf7df2739c9b07efa122

(The below comments were pending comments that I had on earlier versions of your patch, but I've left them here for posterity.)

collinjackson · 2020-01-24T22:24:38Z

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

+///
+/// Once committed, no further operations can be performed on the [WriteBatch],
+/// nor can it be committed again.
+class WriteBatch extends WriteBatchPlatform {


Noting this for posterity since you pushed back on my comment here earlier. What I was trying to suggest was following the same pattern established by Query, QuerySnapshot, separating the public API and method channel implementations. This enables the app-facing class to be extended in the future.

collinjackson · 2020-01-24T22:32:19Z

...store/cloud_firestore_platform_interface/lib/src/platform_interface/field_value_factory.dart

+
+/// An interface for a factory that is used to build [FieldValue] according to
+/// Platform (web or mobile)
+abstract class FieldValueFactory {


In case anyone is wondering why we do verifyExtends... previously this class didn't extend PlatformInterface, making it possible to use implements rather than extends on this class. This could lead to a situation where no new methods could be added to FieldValueFactory without breaking platform implementations. And this wasn't just a hypothetical concern, it happened in the web implementation.

https://github.com/FirebaseExtended/flutterfire/blob/b09cd242d1479d47e84b31265b6722f6ff35b95c/packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/method_channel_field_value_factory.dart#L5

It's fixed now.

collinjackson · 2020-01-24T23:05:23Z

...tore/cloud_firestore_platform_interface/lib/src/platform_interface/collection_reference.dart

+/// A CollectionReference object can be used for adding documents, getting
+/// document references, and querying for documents (using the methods
+/// inherited from [Query]).
+abstract class CollectionReference extends Query {


What I was trying to say here was that this class is tempting to implement using implements (and in fact the original PR https://github.com/FirebaseExtended/flutterfire/pull/1670/files#diff-167cff67d9c16a4fbee5de8f8a9cd33aR4) did so.

… firestore_web/platform_interface

amrfarid140 · 2020-02-05T20:30:55Z

@collinjackson Merged that latest of flutterfire/master to my fork and updated both PRs. Not sure if your comments are based on old changes or the most recent ones.

Also not sure what's the issue with Android builds. Seems CI related rather than changes related. Would be great if you or @ditman can have a look. Thanks.

ditman · 2020-02-05T21:31:37Z

@amrfarid140 I'm relaunching the tests, they seemed to be temporary failures, unrelated to the branch. (Update: Everything is green now.)

collinjackson

ditman · 2020-02-05T21:45:57Z

Tagged v1.0.0, and published to pub.dev: https://pub.dev/packages/cloud_firestore_platform_interface/versions/1.0.0

IchordeDionysos · 2020-02-22T22:14:01Z

@ditman @amrfarid140 May I ask why there is still a platform channel implementation in the cloud_firestore_platform_interface? 🤔

Is it to maintain backward compatibility until all platforms use the platform interface?
To me this is confusing... 😄
I would expect the platform implementation to throw an error that the current platform is not supported and that they can extend support themselves by implementing the platform interface. This would give a better error message than this #1972 and tell developers what's the underlying error. 🤔

ditman · 2020-02-24T20:30:43Z

@IchordeDionysos the MethodChannel implementation is the one used by the "native" platforms (ios/macos/android). Notice how that implementation extends its corresponding PlatformInterface, and is the default implementation.

Check the doc about plugin federation for much much more info!

Created firestore platform interface

0803aef

googlebot added the cla: yes label Dec 20, 2019

amrfarid140 added 2 commits December 21, 2019 00:37

Fixes for multiple apps support

3c57adb

Fixed format issues

e309a8e

ditman requested review from ditman, collinjackson and harryterkelsen December 21, 2019 01:16

ditman requested a review from amirh December 21, 2019 01:21

ditman reviewed Dec 21, 2019

View reviewed changes

amrfarid140 added 3 commits December 21, 2019 17:12

Fix lint issues and remove analysis_options.yml

0c7ccf1

Fix typo

d1898fd

Fix analyze issues

6ce4a4b

ditman reviewed Dec 23, 2019

View reviewed changes

...ages/cloud_firestore/cloud_firestore_platform_interface/lib/src/firestore_message_codec.dart Outdated Show resolved Hide resolved

ditman reviewed Dec 23, 2019

View reviewed changes

amrfarid140 and others added 2 commits December 23, 2019 07:50

Added visiblefortesting annotation

872959d

Appease analyzer

41d5117

Analyzer is complaining about this extra carriage return.

collinjackson reviewed Jan 7, 2020

View reviewed changes

amrfarid140 and others added 8 commits January 8, 2020 08:47

use instanceFor

e53dad1

Apply suggestions for cloud_firestore_platform_interface

a6b319b

Co-Authored-By: Collin Jackson <[email protected]>

Merge remote-tracking branch 'origin/firestore_web/platform_interface…

5dea377

…' into firestore_web/platform_interface

add visible for testing annotation on channel

89d0ddb

Apply suggestions from code review

b0e5c5e

Co-Authored-By: Collin Jackson <[email protected]>

Merge remote-tracking branch 'origin/firestore_web/platform_interface…

49abc85

…' into firestore_web/platform_interface

Formatting fixes

41e9d92

make transaction finish as private

8747a19

ditman approved these changes Jan 17, 2020

View reviewed changes

Added latest changes from @ditman and @collinjackson

8933d26

collinjackson self-requested a review February 5, 2020 15:38

collinjackson reviewed Feb 5, 2020

View reviewed changes

Merge branch 'master' of github.com:FirebaseExtended/flutterfire into…

e09e384

… firestore_web/platform_interface

collinjackson self-requested a review February 5, 2020 21:36

collinjackson approved these changes Feb 5, 2020

View reviewed changes

ditman merged commit 29eb495 into firebase:master Feb 5, 2020

This was referenced Feb 6, 2020

[cloud_firestore] migrate to platform interface #1945

Merged

[cloud_firestore] created firestore web plugin #1950

Merged

jmagman mentioned this pull request Mar 13, 2020

Fix for UserAgent.h issues #2169

Closed

firebase locked and limited conversation to collaborators Aug 5, 2020

[cloud_firestore] Created firestore platform interface #1686

[cloud_firestore] Created firestore platform interface #1686

Uh oh!

Conversation

amrfarid140 commented Dec 20, 2019

Description

Related Issues

Checklist

Breaking Change

Uh oh!

amrfarid140 commented Dec 20, 2019

Uh oh!

ditman commented Dec 21, 2019

Uh oh!

ditman commented Dec 21, 2019

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

collinjackson Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

amrfarid140 Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

collinjackson Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

amrfarid140 Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grundid commented Jan 7, 2020

Uh oh!

ditman commented Jan 7, 2020

Uh oh!

ditman commented Jan 17, 2020

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

amrfarid140 commented Jan 28, 2020

Uh oh!

ditman commented Jan 29, 2020

Uh oh!

amrfarid140 commented Jan 29, 2020 via email

Uh oh!

ditman commented Feb 3, 2020

Uh oh!

ditman commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amrfarid140 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman commented Feb 4, 2020

Uh oh!

amrfarid140 commented Feb 4, 2020

ditman left a comment •

edited

Loading

collinjackson left a comment •

edited

Loading

ditman commented Feb 3, 2020 •

edited

Loading

amrfarid140 commented Feb 3, 2020 •

edited

Loading

ditman commented Feb 5, 2020 •

edited

Loading

collinjackson commented Feb 5, 2020 •

edited

Loading

collinjackson left a comment •

edited

Loading

collinjackson Jan 24, 2020 •

edited

Loading

collinjackson Jan 24, 2020 •

edited

Loading

collinjackson Jan 24, 2020 •

edited

Loading

ditman commented Feb 5, 2020 •

edited

Loading

collinjackson left a comment •

edited

Loading

ditman commented Feb 24, 2020 •

edited

Loading