Request notification permission in getToken if permission is "default" #2421
Conversation
Instead of returning null and expecting the user to request the notification permission, do it automatically. Also some (not enough) refactoring of tests.
| permission = await this.requestNotificationPermission(); | ||
| } | ||
|
|
||
| if (permission !== 'granted') { |
There was a problem hiding this comment.
Wouldn't it be a breaking change for users who have special handling for null? Perhaps they want to show a different message when user cancels the popup, although I don't know how often people do it.
Perhaps a safer option is keep returning null for default?
There was a problem hiding this comment.
I thought about that too, but I'm pretty sure that like 95% of our users just copy paste what we do in quickstart or our guides, which is requesting permission and calling getToken if that succeeds, which would prevent this change from ever affecting them. And if people were calling getToken without making sure they have permission, their program wouldn't have worked anyway.
Besides, previously default meant that the developer probably forgot to ask for permission, but since getToken will do that automatically, default here means "we displayed the permission dialog to the user and they dismissed it without clicking allow or deny", which is much closer to the throw behavior before (denying explicitly).
So if a developer were handling null by requesting permission, if we still return null when the user dismisses the popup, that would make the developer request permission for a second time, which doesn't seem like a good idea to me.
There was a problem hiding this comment.
That makes sense , but wouldn't this change make us request permission for a second time if the developer followed the quickstart where they call Notification.requestPermission() before calling getToken(), and the user dismisses the dialog?
I don't know how I feel about releasing it without a major version bump. FCM is one of the most popular SDKs, so it could affect a lot of people. What is the motivation for this change?
There was a problem hiding this comment.
Discussed offline, this change would not affect apps that followed the quickstart, because they won't call getToken() if user dismissed the dialog in the first place.
There was a problem hiding this comment.
Actually, this change affects functions.httpsCallable. Since it aquires FCM token automatically, when calling some function by httpsCallable, the browser asks user for notification permission.
So, there are cases when the user does not use FCM but uses Clould Functions and get a dialog asking to allow notifications, unecessarlly
There was a problem hiding this comment.
I've just ran into this issue pointed out by @VagnerGon, I had an httpsCallable which doesn't require a user to be logged in, but on executing the function it auto pops up the notification permission on page load which is really not best practice.
This is a breaking change and I hope it can be revisited to not call getToken for httpsCallable, perhaps pass an option to httpsCallable so it returns null if permission is default
There was a problem hiding this comment.
Yeah I didn't realize Functions was doing that, sorry about that. This was fixed in #2489.
Instead of returning null and expecting the user to request the notification permission, do it automatically.
Also some (not enough) refactoring of tests.