[cloud_firestore] Added Flutter web support

amrfarid140 commented 4 years ago

Description

Created cloud_firestore_platform_interface to host A common platform interface for the cloud_firestore plugin.
Created cloud_firestore_web to host the web implementation for Firestore
Changed cloud_firestore implementation to delegate to method_channel_xyz in cloud_firestore_platform_interface when necessary
Relocated tests from cloud_firestore to cloud_firestore_platform_interface

This PR is created for gathering feedback on the approach and check mergeability.

Todo items This PR is created for gathering feedback on the approach. The remaining steps are

[x] create PR and merge cloud_firestore_platform_interface
[x] create PR and merge cloud_firestore_web
[x] update cloud_firestore to use published version of cloud_firestore_web & cloud_firestore_platform_interface
[x] Add unit tests for cloud_firestore_web
[x] Add unit tests to verify that cloud_firestore delegates method calls to delegate
[x] Add unit tests for cloud_firestore_platform_interface
[x] Address PR comments and feedback
[x] Delay FirebasePlatform._instance initialisation until it's called for the first time (lazy init)
[x] Handle encoding/decoding DocumentReference
[x] Fix Query method delegation (currently broken ☹️)
[x] Add support for Transaction
[x] Add support for WriteBatch
[x] Web & mobile specific implementation for GeoPoint
[x] Web & mobile specific implementation for Blob
[x] Fix settings bug mentioned here https://github.com/FirebaseExtended/flutterfire/pull/1670#issuecomment-567307817

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.

[x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[x] If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
[x] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
[x] All existing and new tests are passing.
[x] I updated/added relevant documentation (doc comments with ///).
[x] The analyzer (flutter analyze) does not report any problems on my PR.
[x] I read and followed the [Flutter Style Guide].
[x] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
[x] I updated CHANGELOG.md to add a description of the change.
[x] I signed the [CLA].
[x] I am willing to follow-up on review comments in a timely manner.

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).
[x] No, this is not a breaking change.

collinjackson commented 4 years ago

Looping in @ditman @hterkelsen who have been working on similar functionality in https://github.com/FirebaseExtended/flutterfire/pull/1633

ditman commented 4 years ago

Wow, thanks for doing this! I'm taking a look!

amrfarid140 commented 4 years ago

No probs @ditman 👍 . It might fail builds on CI as packages such as cloud_firestore_web & cloud_firestore_platform_interface are not published yet. If we change it to relative paths instead of versions then it works. I am using this fork on my personal app at the moment.

The only thing that I am not unsure about is Transaction delegation work. As mentioned in the PR details, I am still in the process of adding tests for the new functionality.

Let me know if you have anything.

ditman commented 4 years ago

It might fail builds on CI as packages such as cloud_firestore_web & cloud_firestore_platform_interface are not published yet.

@amrfarid140 Yes, there's also some failures on analyze and format that need to be fixed in the PR, those are related with the coding style (but for example, dartfmt is an automated fix).

The toughest part to fix analyze is going to be (probably) adding documentation to every public member.

The only thing that I am not unsure about is Transaction delegation work. As mentioned in the PR details, I am still in the process of adding tests for the new functionality.

What I did in PR was to leave the tests in the core package of the plugin, slightly modified to run with the new interface and make sure everything works (similar to the changes you have done when you moved the tests from the core to the platform_interface package.

(I wasn't close to working in the Web plugin, so I didn't have a good story to test the web version!)

(BTW, feel free to email me directly: dit at google if you need anything from me, I have time to help you with this).

ditman commented 4 years ago

@amrfarid140 I cloned your fork, and modified it a little bit to run the example of the cloud_firestore package and it loaded! However when clicking on the + icon I got an exception.

The error comes from the jsify util in package:firebase.

It seems that the web types (maybe: 'DocumentReference', 'FieldValue', 'Blob', 'GeoPoint') need to extend the Firestore package types as well, look at _jsify implementations here.

Uncaught (in promise) Error: Invalid argument (dartObject): Could not convert: Instance of 'FieldValue'
    at Object.throw_ [as throw] (errors.dart:196)
    at Object.jsify (utils.dart:120)
    at utils.dart:94
    at IdentityMap.from.forEach (linked_hash_map.dart:23)
    at Object.jsify (utils.dart:93)
    at firestore.DocumentReference._fromJsObject.set (firestore.dart:348)
    at firestore_web.DocumentReferenceWeb.new.setData (document_reference_web.dart:11)
    at cloud_firestore.DocumentReference.__.setData (document_reference.dart:47)
    at cloud_firestore.CollectionReference.__.add (collection_reference.dart:50)
    at add.next (<anonymous>)
    at runBody (async_patch.dart:86)
    at Object._async [as async] (async_patch.dart:125)
    at cloud_firestore.CollectionReference.__.add (collection_reference.dart:48)
    at main.MyHomePage.new._addMessage (main.dart:65)
    at _addMessage.next (<anonymous>)
    at runBody (async_patch.dart:86)
    at Object._async [as async] (async_patch.dart:125)
    at main.MyHomePage.new.[_addMessage] (main.dart:64)
    at _InkResponseState.new.[_handleTap] (ink_well.dart:705)

So close!

Screen Shot 2019-12-17 at 4 36 39 PM

amrfarid140 commented 4 years ago

Thanks for all feedback @ditman .. it's really helpful. I am happy to keep working at this PR to get it merged. I will start by looking into the issue with jsify then address your comments before finally adding tests to all the new items and fixing the analyze & format issues.

amrfarid140 commented 4 years ago

The FieldValue issue should be resolved now. Moving into other parts of the feedback.

ditman commented 4 years ago

Hey @amrfarid140, some extra stuff regarding testing!

I have played a little bit with transactions, following the Firebase for Flutter codelab. Here's the finished app that you should be able to clone and flutter run -d chrome; if all goes according to plan, when you try to vote on a baby name, it'll throw some errors/assertions:

Type '_Future<dynamic>' should be '_Future<Map<String, dynamic>>' to implement expected type 'Future<Map<String, dynamic>>'.
...
========================================
Uncaught (in promise) Error: Assertion failed: org-dartlang-app:///packages/cloud_firestore_web/transaction_web.dart:19:12
documentReference is DocumentReferenceWeb
is not true
    at Object.throw_ [as throw] (errors.dart:196)
    at Object.assertFailed (errors.dart:26)
    at firestore_web.TransactionWeb.__.get (transaction_web.dart:19)
    at get.next (<anonymous>)
    at runBody (async_patch.dart:86)
    at Object._async [as async] (async_patch.dart:125)
    at firestore_web.TransactionWeb.__.get (transaction_web.dart:18)
    at cloud_firestore.Transaction.__.get (transaction.dart:19)
   ...
========================================
Uncaught (in promise) Error: TypeError: Cannot read property 'then' of undefined
...

(I'm sure they're all related to the DocumentReferenceWeb assertion, which ties in with the jsify discussion above)

The other bit of news, after discussing with @collinjackson about testing, is that there's a bunch of integration tests inside the cloud_firebase example app, here.

I've managed to run them in the web by copying the contents of test_driver/cloud_firestore.dart into lib/main.dart, and then running flutter run -d chrome in the example app; here's the current output, and an excerpt:

FirebaseError: [code=invalid-argument]: Function settings() requires its host option to be of type non-empty string, but it was:
null
https://www.gstatic.com/firebasejs/7.5.0/firebase-firestore.js 1:47893                    new Gr
  https://www.gstatic.com/firebasejs/7.5.0/firebase-firestore.js 1:50392                    ci
  https://www.gstatic.com/firebasejs/7.5.0/firebase-firestore.js 1:49519                    oi
  https://www.gstatic.com/firebasejs/7.5.0/firebase-firestore.js 1:332640                   new Wp
  https://www.gstatic.com/firebasejs/7.5.0/firebase-firestore.js 1:334921                   settings
  package:firebase/src/firestore.dart 119:16                                                settings
  package:cloud_firestore_web/firestore_web.dart 65:20                                      <fn>
  package:dart-sdk/lib/async/future.dart 224:31                                             sync

(Looks like an initialization issue is masking the real execution of the tests. (I'll tinker with this some more and add another dump of the output to this issue if I get the tests to run past that settings explosion).

Collin also mentioned that If you add tests there, they'll also help the mobile implementation of the plugin.

Again, thanks for all the work you're doing here!

amrfarid140 commented 4 years ago

@ditman I have sorted out transactions for both Web and Mobile. It should be working now. I've also added a button test them on the example app. Unit tests for it is still pending though.

amrfarid140 commented 4 years ago

Added WriteBatch support and included examples in example app. Runs on Web and Mobile. However, it always fails if we do the following

// get a document reference `docRef`
batch.update(docRef, {some_data})
batch.delete(docRef)
await batch.commit()
...

Basically if we update something then delete it (which maybe be pointless).. Firebase throws an internal error on all platforms

INTERNAL ASSERTION FAILED: Transform results missing for TransformMutation

@ditman Is that intended behaviour ?

amrfarid140 commented 4 years ago

On another news .. format and analyze now are passing 🎆

amrfarid140 commented 4 years ago

@ditman Ran the tests you mentioned here and it pointed out some bugs. Those bugs are fixed now but there are a couple of tests that I wasn't able to run/verify due to missing or insufficient permission. you can see results here.

ditman commented 4 years ago

@amrfarid140 I'm looking at the documentation of batched writes here, and it seems they do update and delete on different document refs... I'll try to dig deeper in the flutterfire discord and get back to you (lmk if you want to join, I can send you an invite there).

Regarding the lack of permissions in tests, let me try and run them and see what's up with that; it might be an authentication issue (or lack of it thereof). I'll try to get that fixed/explained so they don't block you.

I'll take a look at the latest code changes!

amrfarid140 commented 4 years ago

Thanks @ditman that it would great if we can get to bottom if the WriteBatch issue. Also, yeah an invite to flutterfire discord would be helpful 😄 .

ditman commented 4 years ago

@amrfarid140 at this point I feel like I'm nitpicking rather than adding anything smart to the discussion, other than the initialization problem, I think this is looking great and almost ready to go! What do you think Amr?

Also, Discord link in this doc, ping me in there!

amrfarid140 commented 4 years ago

I am fairly happy with how it looks. I've kept testing different use cases with the example and satisfied that all is 💯 . Next steps would be address those latest comments then adding unit tests all over.

Not sure how can pass the publishable test though @ditman . Packages rely on paths right now for everything to work which ultimately is not what we want. We also, at this point, can't rely on specific versions since cloud_firestore_web & cloud_firestore_platfrom_interface are not published yet. What do you think is the best approach for this?

ditman commented 4 years ago

We also, at this point, can't rely on specific versions since cloud_firestore_web & cloud_firestore_platfrom_interface are not published yet. What do you think is the best approach for this?

@amrfarid140 you found The Big Chore of this type of migrations which is that we need to split this one in 3 separate PRs to merge (and publish to pub.dev) the packages in the right order:

platform_interface (which shouldn't depend on anything else)
core_plugin (which only depends on platform interface, but since it's already published we can already depend on the right version)
web_implementation (which may depend on both of the above, which are published by now)

Our tooling is not smart enough to publish multiple packages at the same time :/

amrfarid140 commented 4 years ago

No problem. Let's get those new packages documented and unit tested then I will close this PR and open 3 PRs sequentially (1. Platform interface, 2. Web, 3. Firestore Core) to get it all published. Thanks so much @ditman for the feedback.

ditman commented 4 years ago

No problem. Let's get those new packages documented and unit tested then I will close this PR and open 3 PRs sequentially (1. Platform interface, 2. Web, 3. Firestore Core) to get it all published. Thanks so much @ditman for the feedback.

Sounds good, I'm currently trying to:

Figure out what's up with the WriteBatch (I asked an internal question, but the team may be in a different TZ)
See what's up with the permissions in the example tests, and run those again.
Try to port another codelab to web with this :)

I've also closed most of the conversations above that were already addressed. I don't think there's too many open (other than the ensureInitialized thing which I'd be happy with a bug + comment stating why that line is needed and TODO: remove once bug ID is closed?)

ditman commented 4 years ago

Still waiting for an answer on the WriteBatches
Ran the example/test_driver tests again, there's good news and bad news!
- Good news is that the same 2 tests seem to be failing both on android master and web.
- Bad news is that there's more example tests failing on our work branch on android. These are the new failing tests:
- Firestore getDocumentsFromCollectionGroup [E]
- Firestore pagination [E]
- Firestore pagination with map [E]

(Maybe it's an issue with "where" clauses?)

I had to do 2 small changes to the latest code that we've reviewed (but I can't send PRs to your fork :/)... These are needed so the example tests in web come back to only the 2 ones that were failing because of permission denied:

I recently got permission to access the firebase backend that is used by the tests, where I'll try to review what's up with these two tests, but permissions seem to not have taken effect yet. I'll check again tomorrow in my AM.

Again, thank you very much for all of your work!

amrfarid140 commented 4 years ago

@ditman managed to run tests on mobile with this output.. most of them pass except some items where it needs indices defined or permissions.

I've also added your changes to it and resolved remaining outstanding comments

ditman commented 4 years ago

@ditman managed to run tests on mobile with this output.. most of them pass except some items where it needs indices defined or permissions.

@amrfarid140 I ran the tests in the origin/master branch, and only the 2 tests that fail in web fail there. This 3 tests were broken (somehow) by this PR:

Firestore getDocumentsFromCollectionGroup [E]
Firestore pagination [E]
Firestore pagination with map [E]

I've been reading a little bit, and stumbled upon this StackOverflow answer... I don't see anything that changed that might affect that, other than how the query may be composed? Are we adding "where" clauses multiple times? I think those 3 tests are pointing to a bug (or at least a change in behavior that might affect other users of the plugin).

amrfarid140 commented 4 years ago

Are we adding "where" clauses multiple times? I think those 3 tests are pointing to a bug (or at least a change in behavior that might affect other users of the plugin).

I actually think we might be doing that. We are always resetting the Query on Web. We most probably need to that on mobile as well.

ditman commented 4 years ago

We are always resetting the Query on Web. We most probably need to that on mobile as well.

That can explain why the tests pass on web Just Fine ™, but not on the MethodChannel implementation.

(Currently looking at what's the difference between our getDocumentsFromCollectionGroup and the one in master, I'm setting up Android Studio to debug what flows through the method channel)

ditman commented 4 years ago

I've debugged the getDocumentsFromCollectionGroup (by placing a breakpoint in the invokeMapMethod implementation of the platform_channel, didn't get one to work in the Java side).

It seems we're passing the wrong "app" name to firestore. Here's the good and bad messages that are being sent through the MethodChannel:

Good

good-call

Bad

bad-call

Everything else seems all right!

I'm going to do the same with the other 2 failing tests, to see if there's any clear difference like the one above.

amrfarid140 commented 4 years ago

Yeah.. I have just been reworking things in cloud_firestore & cloud_firestore_platform_interface to fix this and a same issue with runTransaction test.

ditman commented 4 years ago

Yeah.. I have just been reworking things in cloud_firestore & cloud_firestore_platform_interface to fix this and a same issue with runTransaction test.

@amrfarid140 do you want me to debug the other two tests, or are you on top of them?

amrfarid140 commented 4 years ago

@ditman do you mean those two whereArrayContainsAny & whereIn ? .. if they are, then nope I am not on them.

The only other one failing now is runTransaction, which I am currently investigating. I will tidy up and make a commit for the latest changes.

ditman commented 4 years ago

@ditman do you mean those two whereArrayContainsAny & whereIn ? .. if they are, then nope I am not on them.

The only other one failing now is runTransaction. I will tidy up and make a commit for the latest changes.

@amrfarid140 I meant:

Firestore pagination [E]
Firestore pagination with map [E]

For whereArrayContainsAny & whereIn, I need access to the firestore project (which I haven't gotten yet, grrr), but those seem to fail on all platforms, so I'm a little bit less concerned about them 😛

amrfarid140 commented 4 years ago

@ditman Ah, those two are fixed now .. updates pushed if you want to have a look.

amrfarid140 commented 4 years ago

All driver tests pass now @ditman on mobile and web

ditman commented 4 years ago

All driver tests pass now @ditman on mobile and web

@amrfarid140 you're a beast! I'll take a final look and then we'll go on with the 3 PRs to merge this in order. Again, thanks for the contribution, and the patience!

:applause.gif:

ditman commented 4 years ago

Thanks for backporting the fixes from the platform_interface PR to this one, and the web unit tests. Looking great!

ialhashim commented 4 years ago

I faced an issue with Timestamp being defined in both packages cloud_firestore and cloud_firestore_platform_interface. I resolved it by commenting out part 'src/timestamp.dart' in cloud_firestore.dart and adding Timestamp to the export line.

Not sure if that's the best fix but you should test for Timestamp type.

amrfarid140 commented 4 years ago

@ialhashim Are you depending on both cloud_firestore & cloud_firestore_platform_interface?

The only one you need is cloud_firestore which exposes a Timestamp class that's compatible with Firebase type that you are going to use.

ialhashim commented 4 years ago

@amrfarid140 this is the error I am getting

Exception has occurred.
_TypeError (type 'Timestamp' is not a subtype of type 'Timestamp' where
  Timestamp is from package:cloud_firestore_platform_interface/cloud_firestore_platform_interface.dart
  Timestamp is from package:cloud_firestore/cloud_firestore.dart
)

It happens for a Timestamp property being assigned from a returned document to a custom class I made. I am only including cloud_firestore but cloud_firestore includes cloud_firestore_platform_interface.

Also, digging into it I know see that the same field from cloud store comes back as DateTime on the web and Timestamp on mobile.

ditman commented 4 years ago

Also, digging into it I know see that the same field from cloud store comes back as DateTime on the web and Timestamp on mobile.

@ialhashim @amrfarid140 I've seen the same, is it a problem of this change, or the underlying library?

ialhashim commented 4 years ago

In the mean time I had to write a runtimeType check and do a conversion which is a bit ugly.

ditman commented 4 years ago

(Found the issue/PR where Timestamp support is being added to firebase-dart: https://github.com/FirebaseExtended/firebase-dart/pull/208)

amrfarid140 commented 4 years ago

@ditman @ialhashim This might be caused by cloud_firestore_platform_interface/firestore_message_code.dart building cloud_firestore_platform_interface/timestamp.dart when readValueOfType() is executed.

We can fix that by using the same delegation method , similar to what we do for DocumentReference, in cloud_firestore/timestamp.dart and add some logic in cloud_firestore/codec_utility.dart to make sure that we are passing the correct type for reads and writes.

I also think that the issue on firebase-dart is something else. It might not have a direct impact but it will produce inconsistency when devs are expecting Timestamp but instead get a DateTime when running on Web.

amrfarid140 commented 4 years ago

Actually we can also do as @ialhashim mentioned. Delete cloud_firestore/timestamp.dart and export cloud_firestore_platform_interface/timestamp.dart .. we do that for GeoPoint and it works well. What do you think @ditman ?

ditman commented 4 years ago

@amrfarid140 I think the problem happens when we're assigning from the document snapshot to user's data structures (like here).

In web you get a DateTime object, but in Android you get a Timestamp, so you probably need some logic there to assign (similar to the .toDouble() on ints!).

Right now, by the time we get the data from the firestore-dart package, it's already converted to DateTime on web, right?

(Also, wasn't Timestamp already in the platform_interface package?)

ditman commented 4 years ago

(Just checked the Firebase Supported Data Types and there's no distinction between DateTime or Timestamp, so maybe the codec_utility solution is enough, just return a Timestamp when we see a DateTime coming from the data?)

@amrfarid140 what do you think is the best solution? I don't have any specific preference here :)

amrfarid140 commented 4 years ago

@ditman agreed. This DateTime is a problem but I think it's different from what @ialhashim is describing. We currently have two Timestamp definitions, one in base and one in platform interface, and that might be the issue he is mentioning.

The DateTime problem need some other piece of work, not sure what yet 😅 , since firestore_message_codec already makes a distinction between them.

ditman commented 4 years ago

@amrfarid140 Ah, I know why we're making the distinction; it's probably to preserve the types of the data that the user is encoding (imagine that you're storing a big map where you're storing DateTime or Timestamp objects, that information needs to be preserved when reading back from the DB).

However in firestore itself, when you define your collection, you cannot create anything other than a Timestamp (I just checked). And for this one, I think we can't let firestore-dart do the conversion automatically (because then we lose information provided by the user?)

So I guess you're right, there's 2 problems here :)

amrfarid140 commented 4 years ago

It looks like firebase-dart converts Timestamp to DateTime when reading from document, see here.

And it also converts from DateTime to Timestamp when writing to document, see here.

So I think it might make sense if we catch any DateTime in cloud_firestore_web/utils/codec_utility.dart when reading from document and convert to Timestamp. That way we can maintain the same return value across mobile and web.

Kab1r commented 4 years ago

Error occurs when creating as WriteBatch using Firestore.instance.batch();

Error: Expected a value of type 'WriteBatch', but got one of type 'WriteBatchWeb'

amrfarid140 commented 4 years ago

@KabirKwatra The issue should be fixed now. Give it another go and let me know if you get anymore issues. Thanks !

ditman commented 4 years ago

@KabirKwatra The issue should be fixed now. Give it another go and let me know if you get anymore issues. Thanks !

Write Batches are working for me now (both in web and mobile)

rodydavis commented 4 years ago

Thank you so much for this PR! Really looking forward to this 😎

firebase / flutterfire