fix(firebase_auth): Move communication to EventChannels #4643

ened · 2021-01-12T21:45:04Z

Description

Similar to #4209, this PR addresses some issues rgd. isolate communication pertaining left-open "channels" to communicate back from native side to Dart.

Instead, the Flutter EventChannel mechanism is now used for the following parts:

ID Token Changes
Auth State changes
Phone Number verification

Related Issues

Closes #3507
Closes #4416

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:
  firebase_auth:
    git:
      url: https://github.com/ened/flutterfire.git
      ref: fix/4416-firebase-auth
      path: packages/firebase_auth/firebase_auth
  firebase_auth_platform_interface:
    git:
      url: https://github.com/ened/flutterfire.git
      ref: fix/4416-firebase-auth
      path: packages/firebase_auth/firebase_auth_platform_interface

into fix/4416-firebase-auth

packages/firebase_auth/firebase_auth/ios/Classes/FLTFirebaseAuthPlugin.m

ened · 2021-04-06T05:43:06Z

@rrousselGit @Salakar @Ehesp hi there - this PR is ready for review.

The e2e example currently fails due to the phone number being linked with another account. The same error can be observed on the master branch.

Could you spare a minute to take a look?

...th/firebase_auth_platform_interface/lib/src/method_channel/method_channel_firebase_auth.dart

russellwheatley

A few nitpicks from me. One other thing would be to run melos run format from root as I think there is one iOS file that needs formatting. Other than that, great work again, @ened. We really appreciate it.

russellwheatley · 2021-04-06T13:42:44Z

...e_auth/android/src/main/java/io/flutter/plugins/firebase/auth/FlutterFirebaseAuthPlugin.java

@@ -1377,9 +1216,9 @@ public void onMethodCall(@NonNull MethodCall call, @NonNull Result result) {
    return Tasks.call(
        cachedThreadPool,
        () -> {
-          removeEventListeners();
+          // TODO: not sure if this is applicable here.
+          //          removeEventListeners();


@Salakar, will we need to clean up event listeners for didReinitializeFirebaseCore?

When does didReinitializeFirebaseCore get called usually?

Ah, when the core is reinitialized (doh!) - in the firebase core plugin. If the user changes the firebase project then we need to clean up the plugins. There is a problem in cloud_firestore as well as this PR: The didReinitializeFirebaseCore does not clear the event listeners, which may cause errors. The listeners get cleared up when the plugin is destroyed. Hm..

Sounds like a central cleanup method in the plugin would be the right way to go here.

didReinitializeFirebaseCore is also called on Dart Hot Restarts in dev (was mainly added for this purpose) - so it would be best if the event listeners are also cleaned up here otherwise these would linger in native when Dart is no longer aware of them?

...e_auth/android/src/main/java/io/flutter/plugins/firebase/auth/FlutterFirebaseAuthPlugin.java

...oid/src/main/java/io/flutter/plugins/firebase/auth/PhoneNumberVerificationStreamHandler.java

packages/firebase_auth/firebase_auth/ios/Classes/FLTAuthStateChannelStreamHandler.m

packages/firebase_auth/firebase_auth/ios/Classes/FLTPhoneNumberVerificationStreamHandler.m

ened · 2021-04-07T11:27:57Z

...th/firebase_auth_platform_interface/lib/src/method_channel/method_channel_firebase_auth.dart

        'phoneNumber': phoneNumber,
        'timeout': timeout.inMilliseconds,
        'forceResendingToken': forceResendingToken,
        'autoRetrievedSmsCodeForTesting': autoRetrievedSmsCodeForTesting,
      });
+
+      EventChannel(eventChannelName!)


This StreamSubscription is not cancelled.

google-cla · 2021-04-15T11:35:40Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Salakar · 2021-04-15T11:36:47Z

@googlebot I consent.

ened · 2021-04-15T12:01:52Z

Thank you @Salakar for joining :)

Salakar · 2021-04-15T13:19:51Z

Thank you @Salakar for joining :)

No worries, I think this is the last bit and then we're good to merge and ship;

The test failures re credential in use are also occurring on master so unrelated to this PR and being investigated separately.

ened · 2021-04-15T17:31:56Z

Thank you @Salakar for joining :)

No worries, I think this is the last bit and then we're good to merge and ship;

The test failures re credential in use are also occurring on master so unrelated to this PR and being investigated separately.

OK will take care. I did not see your reply before just now.

ened · 2021-04-20T12:14:04Z

@Salakar Addressed the last piece of feedback. Please also see #5852 for the similar fix in cloud_firestore.

Sebastian Roth and others added 13 commits January 7, 2021 07:29

Begin event channel

29cdc29

Add listeners & handler methods on Android

86ef985

Dart todo

a894b32

Merge branch 'master' into fix/4416-firebase-auth

e6a59fa

create id token channel on demand

784f599

WIP

d5e647b

Merge branch 'master' into fix/4416-firebase-auth

efee572

wip

ea31954

Link auth platform from local repo

6989f72

reactivate tests

02b7ae1

Resolve errors

6ccd999

Java formatting

b897f5c

Move remaining parts to EventChannels

7623664

google-cla bot added the cla: yes label Jan 12, 2021

ened changed the title ~~Move Firebase Auth communication to EventChannels when possible~~ [firebase_auth] Move communication to EventChannels Jan 13, 2021

ened changed the title ~~[firebase_auth] Move communication to EventChannels~~ fix(firebase_auth): Move communication to EventChannels Jan 13, 2021

Sebastian Roth added 3 commits January 13, 2021 13:11

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

16eb460

into fix/4416-firebase-auth

iOS implementation

107d817

Correct class name, restore macos links

c8cb7c8

ened commented Jan 14, 2021

View reviewed changes

packages/firebase_auth/firebase_auth/ios/Classes/FLTFirebaseAuthPlugin.m Show resolved Hide resolved

Sebastian Roth added 3 commits January 14, 2021 09:55

Missing file

73a06fb

Ensure not to compile phone number verification code on macOS

9e707ed

cleanups on iOS

3fa9154

ened mentioned this pull request Jan 14, 2021

🐛 [Cloud Firestore] Fatal Exception: java.lang.NullPointerException Attempt to invoke virtual method 'void f.a.b.a.j.a(java.lang.String, java.lang.Object)' on a null object reference #3507

Closed

ened marked this pull request as ready for review January 14, 2021 15:07

Merge branch 'master' into fix/4416-firebase-auth

4998cb9

ened changed the base branch from master to no-nullsafety January 17, 2021 08:01

ened requested review from Ehesp, kroikie and Salakar as code owners January 17, 2021 08:01

ened added 2 commits April 3, 2021 09:00

Merge branch 'master' into fix/4416-firebase-auth

62fe703

Add a workaround for nullable types in stream controller

7965240

rrousselGit reviewed Apr 6, 2021

View reviewed changes

...th/firebase_auth_platform_interface/lib/src/method_channel/method_channel_firebase_auth.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Apr 6, 2021

View reviewed changes

...th/firebase_auth_platform_interface/lib/src/method_channel/method_channel_firebase_auth.dart Outdated Show resolved Hide resolved

russellwheatley requested changes Apr 6, 2021

View reviewed changes

ened added 4 commits April 6, 2021 20:52

iOS feedback

8b834ad

Android feedback

5e5d941

Dart feedback

1923937

Formatting

812610c

ened commented Apr 7, 2021

View reviewed changes

Salakar added 2 commits April 15, 2021 12:20

Merge branch 'master' into fix/4416-firebase-auth

6d04f1a

refactor: fix analyzer warning and optimise imports

08884b0

google-cla bot added cla: no and removed cla: yes labels Apr 15, 2021

google-cla bot added cla: yes and removed cla: no labels Apr 15, 2021

Sebastian Roth added 3 commits April 20, 2021 12:15

Merge branch 'master' into fix/4416-firebase-auth

5bf126f

Clear event listeners when firebase core is re-initialized

0e2ba9b

formatting

1cbf588

Salakar approved these changes Apr 20, 2021

View reviewed changes

Salakar merged commit 8115cd5 into firebase:master Apr 20, 2021

russellwheatley mentioned this pull request Apr 22, 2021

🐛 [firebase_auth] Randomly getting "bad state" on Flutter 1.22 #3756

Closed

firebase locked and limited conversation to collaborators May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(firebase_auth): Move communication to EventChannels #4643

fix(firebase_auth): Move communication to EventChannels #4643

ened commented Jan 12, 2021 •

edited

Loading

ened commented Apr 6, 2021

russellwheatley left a comment

russellwheatley Apr 6, 2021

ened Apr 6, 2021

ened Apr 6, 2021

Salakar Apr 15, 2021

ened Apr 7, 2021

google-cla bot commented Apr 15, 2021

Salakar commented Apr 15, 2021

ened commented Apr 15, 2021

Salakar commented Apr 15, 2021 •

edited

Loading

ened commented Apr 15, 2021

ened commented Apr 20, 2021

fix(firebase_auth): Move communication to EventChannels #4643

fix(firebase_auth): Move communication to EventChannels #4643

Conversation

ened commented Jan 12, 2021 • edited Loading

Description

Related Issues

Checklist

Breaking Change

Please help testing the PR

ened commented Apr 6, 2021

russellwheatley left a comment

Choose a reason for hiding this comment

russellwheatley Apr 6, 2021

Choose a reason for hiding this comment

ened Apr 6, 2021

Choose a reason for hiding this comment

ened Apr 6, 2021

Choose a reason for hiding this comment

Salakar Apr 15, 2021

Choose a reason for hiding this comment

ened Apr 7, 2021

Choose a reason for hiding this comment

google-cla bot commented Apr 15, 2021

Salakar commented Apr 15, 2021

ened commented Apr 15, 2021

Salakar commented Apr 15, 2021 • edited Loading

ened commented Apr 15, 2021

ened commented Apr 20, 2021

ened commented Jan 12, 2021 •

edited

Loading

Salakar commented Apr 15, 2021 •

edited

Loading