[firebase_firestore]: Stream does not load value, but future does on web

Albert-Jan commented 4 months ago

Is there an existing issue for this?

[X] I have searched the existing issues.

Which plugins are affected?

Cloud Firesotre

Which platforms are affected?

Web

Description

After updating Flutter Firestore from version 4.X to 5.X, the web version, the stream no longer emits a value. Downgrading makes the same code work as expected. The future of the same document works without a problem.

Reproducing the issue

Listen to a stream:

Stream<Project> listenProject(String projectId) => _service.doc(FirestorePath.project(projectId)).snapshots().map((DocumentSnapshot<Map<String, dynamic>> data) => Project.fromMap(data.data()!));

Future loads just fine: Future<Project> getProject(String projectId) async => _service.doc(FirestorePath.project(projectId)).get().then((DocumentSnapshot<Map<String, dynamic>> data) => Project.fromMap(data.data()!));

Firebase Core version

3.1.1

Flutter Version

3.22.2

Relevant Log Output

No error is thrown, even when wrapped with try/catch

Flutter dependencies

No response

Additional context and comments

No response

TarekkMA commented 4 months ago

@Albert-Jan Thank you for reporting this issue. I've replicated the code for the example Firestore app in this branch: issue/13019, and I didn't find any issues. Can you try this branch and see if you still have problems?

Albert-Jan commented 4 months ago

Thanks for your response; I updated my pubspec to rely on the cloud_firestore of the custom branch. I also performed a flutter clean to make sure it was properly compiled.

cloud_firestore: git: url: https://github.com/firebase/flutterfire.git ref: issue/13019 # branch name path: packages/cloud_firestore/cloud_firestore

Unfortunately, it did not work as expected, and the same bug happened. I double checked and downgrading to cloud_firestore: 4.17.4 does work as expected without any code changes.

TarekkMA commented 4 months ago

Can you try running the example app found at this branch and test it with the provided configuration? Then, integrate your Firebase configuration into the example app and test again.

russellwheatley commented 4 months ago

Hey @Albert-Jan - I think I was able to reproduce, fairly sure it is the same as this issue: https://github.com/firebase/flutterfire/pull/13029

Albert-Jan commented 4 months ago

I had a quick look. This might be the problem. Is there a branch I can test this against?

russellwheatley commented 4 months ago

@Albert-Jan Sure, do you mind testing this branch to see if it fixes your issue, please? https://github.com/firebase/flutterfire/tree/firestore-13019

Let me know how it goes 🙏

russellwheatley commented 4 months ago

For testing, I think updating pubspec.yaml to something like this ought to work:

dependencies:
  cloud_firestore_web:
    git:
      url: https://github.com/FirebaseExtended/flutterfire.git
      ref: firestore-13019
      path: packages/cloud_firestore/cloud_firestore_web

russellwheatley commented 4 months ago

@Albert-Jan - did you have a chance to test this?

alextekartik commented 4 months ago

I still have the issue where registering twice to the same document simply does not work on the web (in debug mode at least)

It does not work using cloud_firestore_web 5.0.3
It does not work using using git (master or issue/13019)
It works on Android
It works when using cloud_firestore: 4.17.4

In my testing I'm listening to 2 documents in 2 locations. It seems subscribing a second time is cancelling the first one (i.e. no more data received on the first subscriber).

jamesblasco commented 4 months ago

Same as @alextekartik, trying both cloud_firestore_web 5.0.3 and git master does not fix the issue to me. Everything seems work fine on profile/release mode on web.

alextekartik commented 4 months ago

I think the debug map that holds subscribed key only works when listening to the same DocumentReference (i.e. not when listening to two different DocumentReferences pointing to the same path)

A simple code like this should help reproduce this issue.

var path = ...
var ref1 = firestore.doc(path);
var ref2 = firestore.doc(path);
ref1.snapshots().listen((_) => ...);
// This cancels the first listen.
ref2.snapshots().listen((_) => ...);

Basically I don't think _docListeners (and others similar chear for debug) should be store at the DocumentReference level but rather be at the firestore level as the key (ending with 0) will be the same for 2 different refs pointing to the same path.

alextekartik commented 4 months ago

There is a similar issue with collections (i.e. 2 ref pointing to the same path).

I published a PR that fixes the issue here: https://github.com/firebase/flutterfire/pull/13075

If someone else (@jamesblasco) wants to try, you can use the following override:

  cloud_firestore_web:
    git:
      url: https://github.com/tekartik-2/flutterfire
      path: packages/cloud_firestore/cloud_firestore_web

Albert-Jan commented 3 months ago

I just tested with Firestore 5.1.0, and the issue persists; what branch can I test with?

alextekartik commented 3 months ago

@russellwheatley Is there a way to re-open the issue ? This makes it barely usable to debug on the web.

A quick fix would be to store the cache of subscription at the firestore level (instead at the document reference level).

If there was a way to run unit test that is ran in your CI?, I would be happy to write one!

I created a sample program that shows the issue here: https://github.com/tekartikdev/publicexp/tree/cloud_firestore_issue_13019/cloud_firestore_exp

To reproduce issue 13019

Run in debug on a web target.

To see the issue on document:

Setup listener 1
Setup listener 2
increment doc => only listener 2 is called

To see the issue on collection:

Setup listener 3
Setup listener 4
increment doc => only listener 4 is called

Override

To fix the issue, use the following override in your pubspec.yaml:

  cloud_firestore_web:
    git:
      url: https://github.com/tekartik-2/flutterfire
      path: packages/cloud_firestore/cloud_firestore_web

Additional notes

The smart option used to cancel existing listeners on hot restart is not really working. listeners are closed by index so the last one is only disabled when the same index is reached. Instead, all listeners should be closed on hot restart.

To see that it is not working:

Setup listener 1
increment doc => listener 1 is called
Hot restart
increment doc => listener 1 is called (and should not)

russellwheatley commented 3 months ago

@alextekartik - Thanks for the update. I'll take a look at this 👍

russellwheatley commented 3 months ago

@alextekartik PR up if you want to try out that branch: https://github.com/firebase/flutterfire/pull/13119

alextekartik commented 3 months ago

Thank you Russell. Indeed the following overrides fixe the most important issue (I had to override cloud_firestore_platform_interface as well since it was not compiling)

dependency_overrides:
  cloud_firestore_web:
    git:
      url: https://github.com/FirebaseExtended/flutterfire.git
      ref: fix-firestore-hot-reload-web
      path: packages/cloud_firestore/cloud_firestore_web
  cloud_firestore_platform_interface:
    git:
      url: https://github.com/FirebaseExtended/flutterfire.git
      ref: fix-firestore-hot-reload-web
      path: packages/cloud_firestore/cloud_firestore_platform_interface

Not sure what you mean by "PR up". I made some minor comments (i.e. miss the testing of 2 references pointing the same doc or collection which is the original issue) but it looks fine to me.

There is still the other issue of subscriptions not clear on hot-reload (still happening in my test app). I guess this requires a manual test. I think the issue is in firestore_core_web: https://github.com/firebase/flutterfire/blob/master/packages/firebase_core/firebase_core_web/lib/src/interop/utils/utils.dart

In my understanding, on the web, the dart globals are cleared on hot-restart but the javascript globals are not. All the registrations keys and callback are saved at the root window js object (side note: I would rather save in a sub object such as flutterfire-debug). On hot-restart all the javascript saved callbacks should be called to clean all existing registrations that still exists on the javascript side (basically we miss a cleanAllWindowsListeners() that should be called on start in debug mode. By doing so you would not even need to save a map or all registrations but just remove the key (which could be just an increment number shared by all web services). If I'm not clear I could do a PR if you want.

Thank you!

firebase / flutterfire