[firebase_auth_web] _fromJsUser and _fromJsUserInfo null safety

[nstosic-toptal]

First, I'm posting a broader API design question that might make this bug relevant or not:
Is there a particular reason why _fromJsUser and _fromJsUserInfo are not considering whether the input parameter is null? There is a pretty obvious use case in which this is true, which I'll describe in the bug below.

Describe the bug
onAuthStateChange stream throws an uncaught error when user is not authenticated (firebase.User is null).

To Reproduce
Steps to reproduce the behavior:

1. Initialize at least one subscription to FirebaseAuth.instance.onAuthStateChanged stream.
2. Run the application on web platform (i.e. flutter run -d chrome)
3. In case the user is not authenticated or authentication status changes to that state, the console will output the following exception and your subscription(s) won't be triggered:

firebase_auth_web.dart:46 Uncaught (in promise) TypeError: Cannot read property 'providerId' of null
    at firebase_auth_web.FirebaseAuthWeb.new.[_fromJsUser] (firebase_auth_web.dart:46)
    at _MapStream.new.[_handleData] (stream_pipe.dart:229)
    at _ForwardingStreamSubscription.new.[_handleData] (stream_pipe.dart:166)
    at _RootZone.runUnaryGuarded (zone.dart:1316)
    at _BroadcastSubscription.new.[_sendData] (stream_impl.dart:338)
    at _BroadcastSubscription.new.[_add] (stream_impl.dart:265)
    at _SyncBroadcastStreamController.new.[_sendData] (broadcast_stream_controller.dart:377)
    at _SyncBroadcastStreamController.new.add (broadcast_stream_controller.dart:252)
    at auth.dart:307
    at Object._checkAndCall (operations.dart:326)
    at Object.dcall (operations.dart:331)
    at Object.ret [as next] (js_dart2js.dart:531)
    at subscribe.ts:104
    at subscribe.ts:233

Expected behavior
onAuthStateChanged stream should trigger another event with null as the value of the observed FirebaseUser parameter.

Additional context
I'll edit this if there is additional questions in the comments.

For now, here's flutter doctor and flutter --version:

$ flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel beta, v1.12.13+hotfix.6, on Mac OS X 10.14.6 18G1012, locale en-RS)
 
[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 11.3)
[✓] Chrome - develop for the web
[✓] Android Studio (version 3.5)
[✓] VS Code (version 1.41.0)
[✓] Connected device (3 available)

• No issues found!
$ flutter --version
Flutter 1.12.13+hotfix.6 • channel beta • https://github.com/flutter/flutter.git
Framework • revision 18cd7a3601 (9 days ago) • 2019-12-11 06:35:39 -0800
Engine • revision 2994f7e1e6
Tools • Dart 2.7.0

EDIT1: Here's the relevant line that's calling the _fromJsUser with null input param.

[muhleder]

Seeing the same. Here's the simple fix I'm using for _fromJsUser muhleder@fbf9760

[nstosic-toptal]

Hello Mark,

Thanks for the feedback. Yeah, hotfix is quite trivial and obvious. I'd like to know if there was any API design decision about not allowing null values. But if there was, it's in contradiction with the design of onAuthStateChanged subscription, since it has a pretty much regular case of emitting null whenever the user is in a non authenticated state.

I guess we'll wait for one of the maintainers to see which one is going to be changed and in the meantime use the hotfix you mentioned.

[nstosic-toptal]

Tagging code owners @collinjackson @kroikie to get their input.

[yumin-chen]

Encountering the same bug here. This makes the entire onAuthStateChanged API unusable on the web.

[chertov]

I have the same problem

firebase_auth_web.dart:46 Uncaught (in promise) TypeError: Cannot read property 'providerId' of null
    at firebase_auth_web.FirebaseAuthWeb.new.[_fromJsUser] (firebase_auth_web.dart:46)
    at _MapStream.new.[_handleData] (stream_pipe.dart:229)
    at _ForwardingStreamSubscription.new.[_handleData] (stream_pipe.dart:166)
    at _RootZone.runUnaryGuarded (zone.dart:1316)
    at _BroadcastSubscription.new.[_sendData] (stream_impl.dart:339)
    at _BroadcastSubscription.new.[_add] (stream_impl.dart:266)
    at _SyncBroadcastStreamController.new.[_sendData] (broadcast_stream_controller.dart:377)
    at _SyncBroadcastStreamController.new.add (broadcast_stream_controller.dart:252)
    at auth.dart:307
    at Object._checkAndCall (operations.dart:326)
    at Object.dcall (operations.dart:331)
    at ret (js_dart2js.dart:531)
    at auth.js:1457

[collinjackson]

Thanks everyone for your patience here.

Tagging @ditman @hterkelsen who have been driving things forward on the web side.

I think the fix in muhleder@fbf9760 is a reasonable one and will put that up as a PR.

[ditman]

Yep, we need the above sanity check.

I checked the documentation, and even though it doesn't state that user may be null directly, there's an if (user) { check that should have given us a clue.

(currentUser however, does specify that may be null).

@collinjackson do we wait for @muhleder's fix, or should we push a quick change?

[collinjackson]

We should fix it, if you want to put up a PR I'm happy to LGTM it

[ditman]

Ah, I remember. The null check was supposed to be performed before calling the _fromJsUser method, but it isn't.

See the MethodChannel implementation vs the Web implementation.

@collinjackson I'll put up a PR quickly, will follow up via chat.

[ditman]

providerData seems to not be nullable (nor contain nulls), so no need to harden _fromJsUserInfo, apparently.

[ditman]

Posted CR, took me a while to write the test 😅

[nstosic-toptal]

Thanks for fixing it.

By the way (although this is quite off-topic) two of my clients are evaluating web approach with Flutter and their point of view, with which I agree, is that, while the build system and Flutter framework itself is quite stable, the ecosystem is far from it. This includes Firebase which is semi-first-party library.

With that in mind, I'd love to make some contribution for the web support rather than pointing fingers, although ironically, JavaScript is my weakest suit. I came into Flutter from native mobile app development. But, if I find enough time, I'll open at least a couple of PRs before summer.