angular / angularfire

Angular + Firebase = ❤️
https://firebaseopensource.com/projects/angular/angularfire2
MIT License
7.66k stars 2.19k forks source link

AngularFireMessaging requests notifications when added to constructor #2276

Closed zanozbot closed 3 years ago

zanozbot commented 4 years ago

Version info

Angular: 8.2.14

Firebase: 7.6.1

AngularFire: 5.2.3

How to reproduce these conditions

Steps to set up and reproduce Add AngularFireMessaging to the constructor of a service or a component.

Expected behavior

The service should initialize without the need to request for notifications. There should be an option to disable automatic notification request.

From the docs

If you blindly ask for permission you have an extremely high chance of getting denied.

Actual behavior

AngularFireMessaging asks for permission on initialization, resulting in immediate notification request. https://github.com/angular/angularfire/blob/master/src/messaging/messaging.ts

jamesdaniels commented 4 years ago

It seems like FCM might have introduced a breaking API change on a minor https://github.com/firebase/firebase-js-sdk/pull/2421... I’d suggest rolling back your Firebase version for now. It’s also caused a similar effect in httpsCallable if you’re using that, which had to be patched with the release of Firebase 7.6.2.

jamesdaniels commented 4 years ago

Thoughts @mmermerkaya @Feiyang1?

zanozbot commented 4 years ago

@jamesdaniels actually, the newest release of firebase 7.6.2. fixes the issue described. The permission is no longer requested on service initalization.

sidsakhadeo commented 4 years ago

@zanozbot @jamesdaniels I'm seeing the same issue even after upgrading to the 7.6.2 version for Firebase. Have you found another workaround for this issue?

metalllus commented 4 years ago

I get the permission request upon using the getToken() method and also the tokenChanges observable which wasn't the case with older versions of Firebase. Is it somehow possible to get around this?

ayyash commented 4 years ago

i'm at 7.8 and the tokenChanges still fires a permission request, so what I ended it up doing is this

 if (Notification.permission === 'granted') {

                            this.afMessaging.tokenChanges.subscribe(() => {
                                this.afMessaging.getToken.subscribe(token => {
                                    _attn(token, 'afnew token');
                                    this.AddSubscriber(token).subscribe();
                                });
                            });

                        }
metalllus commented 4 years ago

i'm at 7.8 and the tokenChanges still fires a permission request, so what I ended it up doing is this

 if (Notification.permission === 'granted') {

                            this.afMessaging.tokenChanges.subscribe(() => {
                                this.afMessaging.getToken.subscribe(token => {
                                    _attn(token, 'afnew token');
                                    this.AddSubscriber(token).subscribe();
                                });
                            });

                        }

you saved my life, thx man

johanchouquet commented 4 years ago
 if (Notification.permission === 'granted') {

                            this.afMessaging.tokenChanges.subscribe(() => {
                                this.afMessaging.getToken.subscribe(token => {
                                    _attn(token, 'afnew token');
                                    this.AddSubscriber(token).subscribe();
                                });
                            });

                        }

One subcribe, in another subscribe, in another subscribe... Not the best pattern I believe. In a more Rxjs way, that would be something like:

const tokenChanges$: Observable<any> = this.afMessaging.tokenChanges;
const token$: Observable<any> = tokenChanges$.pipe(
switchMap(() => this.afMessaging.getToken.pipe(
tap(token => _attn(token, 'afnew token')),
map(token => this.AddSubscriber(token))
)
)
);
this.tokenSub: Subscription = token$.subscribe(); // don't forget to unubscribe
jamesdaniels commented 3 years ago

this has been addressed on our side in recent patches, there was a breaking change in a minor of the firebase sdk