feat(cloud_firestore): Move Snapshot handling into a EventChannel

[ened]

Description

As discussed in #4108, the current implementation for query snapshots (or document snapshots) is not working well in a multi isolate environment due to a static reference to the MethodChannel that calls back from Native to Dart.

This PR introduces a EventChannel per snapshot listener which works around this problem.

Related Issues

Closes #3671
Closes #4108
Closes #4267

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. Updating the pubspec.yaml and changelogs is not required.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• I signed the CLA.
• 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).
• No, this is not a breaking change.

Please help testing the PR

Add the following to your pubspec.yaml:

dependency_overrides:
  cloud_firestore:
    git:
      url: https://github.com/ened/flutterfire.git
      ref: fix/4108-cloud-firestore
      path: packages/cloud_firestore/cloud_firestore
  cloud_firestore_platform_interface:
    git:
      url: https://github.com/ened/flutterfire.git
      ref: fix/4108-cloud-firestore
      path: packages/cloud_firestore/cloud_firestore_platform_interface

Then run flutter clean && flutter pub get & test your iOS & Android Apps again.
Good test scenarios:

• Schedule a piece of Firestore Work using flutter_workmanager
• Update a Firestore document reference in the background_locator plugin callbacks

[ened]

@Salakar this has spiralled quite a bit.

The channel reference is now transitioned to EventChannel for the 4 uses it had:

• SnapshotInSync / Documents / Query Snapshot (easy stuff - one way communication once there is data)
• Transaction handling 🤯. Lots of back & forth with the platform but seems to work in the sample project.

I have noticed there are no tests for snapshot cancellations (else, I'd have App crashes ;-)) so those should be added at some point. The PR needs a lot more work, but please let me know if it's acceptable to work on above 4 components in the same PR. I prefer PRs to be smaller normally...

[Salakar]

I have noticed there are no tests for snapshot cancellations (else, I'd have App crashes ;-)) so those should be added at some point.

Do you mean in the example app or in the e2e tests? I thought we had some in e2e; https://github.com/FirebaseExtended/flutterfire/tree/master/packages/cloud_firestore/cloud_firestore/example/test_driver - could be wrong though 😅 any additional tests you can add to e2e tests here would be extremely welcome!

The PR needs a lot more work, but please let me know if it's acceptable to work on above 4 components in the same PR. I prefer PRs to be smaller normally...

I don't mind either way. If you feel like you can break it down into smaller PRs that can function independently then be my guest - might make it easier on the review, otherwise a single PR is also fine - CI should cover most things before getting to review anyway.

[Ehesp]

Hey @ened

Thanks for starting this. I do have one question, since a lot of the current implementation was built on-top of the original code, we used MethodChannel pretty much everywhere. It was my understanding that using EventChannel is for sending one-way messages over the bridge, which wouldn't work with Transactions.

Originally, it was sending the result of a Transaction (anything the user returns) back to native, and then native sent that back to Dart land. The problem was, if the user sent a class or something to native you couldn't serialize it over the bridge & it'd throw an error. I noticed you've got a storeResult handler which seems to do this?

In the rework, I kept everything stored in Dart land (see here), and when the transaction finishes, I then pluck that value out and return it as the transaction result (see here). This way, it keeps away from native and the user can return whatever they like.

Sending and storing data on native works for primitive values like numbers and strings, but it'll break when trying to return something else, such as a DocumentReference.

[ened]

Do you mean in the example app or in the e2e tests? I thought we had some in e2e; https://github.com/FirebaseExtended/flutterfire/tree/master/packages/cloud_firestore/cloud_firestore/example/test_driver - could be wrong though 😅 any additional tests you can add to e2e tests here would be extremely welcome!

Essentially, 2 snapshot observers (for the same query or document) should be opened simultaneously and the result of cancel awaited. Will need to double check this. There was definitely a ObjC exception that did not make it up. (or it was not raised, ObjC has some safeguards).

I don't mind either way. If you feel like you can break it down into smaller PRs that can function independently then be my guest - might make it easier on the review, otherwise a single PR is also fine - CI should cover most things before getting to review anyway.

A single PR it is. There is actually quite a bit of overlap on the Dart side which is better to read when in context. Thank you.

[ened]

@Ehesp hi

[...]

In the rework, I kept everything stored in Dart land (see here), and when the transaction finishes, I then pluck that value out and return it as the transaction result (see here). This way, it keeps away from native and the user can return whatever they like.

Sending and storing data on native works for primitive values like numbers and strings, but it'll break when trying to return something else, such as a DocumentReference.

For the non-native types Firestore utilises FirestoreMessageCodec which handles the conversion from Firestore objects like DocumentReferencePlatform to something Flutter can pass to native. There is a decoder on the native side as well, for example in FLTFirebaseFirestoreReader. I think that's a very cool way of utilising message codecs. From my understanding it allows the plugin to pass those native objects around freely between Dart & Native; with automatic conversion.

In your examples only DocumentReference as a type was mentioned; as that type (and any other Firestore type) is covered, I really need to check if runTransaction results can contain other types (say my "SuperDuperObject" class):

I think it is not a problem, let's go through the code.

1. runTransaction is defined as template method with <T>: Therefore we expect any dart object to come back
2. runTransaction creates a StreamController: This means we need the result from that controller.
3. the controller starts the transaction by subscribing to the transaction stream with the correct parameters during onListen and keeping track in a separate subscription. The StreamHandler in iOS starts the transaction, then emits a single map value with a key named attempt
4. The Dart side picks up the attempt, generates the transaction command values and sends it back via storeResult.
5. Meanwhile, the iOS side has been awaiting the semaphore which signals a the transaction commands are ready and processes the commands.
6. The dart side moves on after storeResult is done and emits the result in the streamController.. I believe storeResult is a misleading name - suggestions welcome.

Therefore, the developer defined, non-firebase, objects never cross Dart <-> Native. Only the Firebase objects do.

There is a missing piece with transaction as the transaction here could still fail (NSError*) and the dart side would know nothing about it. I believe this is fixable by returning a appropriate value as storeResult response.

[Ehesp]

@ened Thanks - I'll put some time aside soon to review this however your comments generally make sense. Yeah the SuperDuperObject object problem did exist in the old code, so just wanted to make sure we didn't introduce regression.

I've not properly read through the code yet, only skimmed so I probably missed a few things.

[Salakar]

I can take a look at the auth side of things.

Do you have a estimate? We received crash reports so it may me on my list for next week.

If you can look at Auth also that would be super helpful - our plate is quite full at the moment with NNBD changes so any help with PRs is appreciated. @Ehesp is also mostly out of office now for a few weeks so replies may be slow.

One to keep in mind; on the Auth side of things @mikehardy is working on upgrading native Firebase SDKs on all plugins so once this is done we can switch the auth e2e testing to use the new Firebase Auth Emulator which should reduce flakiness/improve local testing - shouldn't be a blocker for you but may land mid-PR.

---

cc @rrousselGit also for a final review if you can please. My quick glance at native bits and this LGTM (though not fully reviewed). With @russellwheatley & @rrousselGit approval I'm happy to ship

[rrousselGit]

Not blocker but: It's not a good practice to create a new Stream directly inside the build method.

Since we're inside a StatefulWidget, this could be moved as property of State (and ideally same thing for the other streams in build

[rrousselGit]

Maybe snapshotsInSync should be a getter and cached instead, for convenience. But that's a breaking change

[ened]

I don't disagree, however I believe those example projects try to convene the point of how to use the API above best coding practices. The way it's written now requires a developer to only look at a single place to see how the streams are used.

Moving it into initState will then require a dispose() call too

[rrousselGit]

We wouldn't need a dispose as there is nothing to dispose of.

It'd just be:

class MyState extends State<T> {
  final stream = FirebaseFirestore.instance.snapshotsInSync();
}

As an example, I believe it's important to show good practices, as other people will rely on those in their application. If the example uses a bad practice, that may make the bad practice propagate

[ened]

hi @rrousselGit thank you for reviewing. The code has been updated & a major part of the review eliminated by deleting the code in question. ;-)). Please let me know if you have further comments.

[Salakar]

Providing @rrousselGit is happy with the feedback changes I'd like to get this merged today 🙃

After that we need to figure out a plan to fix the other plugins 🙈

[rrousselGit]

A test to verify that the stacktrace is passed correctly in the error streams would be awesome.

But otherwise LGTM

[ened]

@rrousselGit I had a look at forwarding the stack in method_channel_document_reference, however it is empty on the source if you will:

        }, onError: (error, stack) {
          print('add stack: $stack');
          controller.addError(convertPlatformException(error), stack);
        });

outputs ''

From scanning the native code, I can not see a way to push a custom stack trace either.

Therefore I'd say we ignore it for now.

[rrousselGit]

Fair enough

[ened]

After that we need to figure out a plan to fix the other plugins 🙈

@Salakar I'm looking at firebase_auth currently - do let me know if there is another channel to discuss / coordinate.

[Salakar]

Drop me a mail and I can invite you to our internal slack if you'd like. mike [at] invertase.io

[DeanPack]

any idea when this will be available on pub.dev? I'm excited to see if this fixes a lot of crashes