Skip to content

Request notification permission in getToken if permission is "default" #2421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Request notification permission in getToken if permission is "default" #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97bec23
Request notification permission in getToken if permission is "default"
mmermerkaya Dec 6, 2019
ef012a5
Fix comment
mmermerkaya Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/messaging-types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
export class FirebaseMessaging {
private constructor();
deleteToken(token: string): Promise<boolean>;
getToken(): Promise<string | null>;
getToken(): Promise<string>;
onMessage(
nextOrObserver: NextFn<any> | Observer<any>,
error?: ErrorFn,
Expand Down
33 changes: 25 additions & 8 deletions packages/messaging/src/controllers/base-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,16 @@ export abstract class BaseController
this.tokenDetailsModel = new TokenDetailsModel(services);
}

async getToken(): Promise<string | null> {
// Check with permissions
const currentPermission = this.getNotificationPermission_();
if (currentPermission === 'denied') {
async getToken(): Promise<string> {
// Check notification permission.
let permission = this.getNotificationPermission();
if (permission === 'default') {
// The user hasn't allowed or denied notifications yet. Ask them.
permission = await this.requestNotificationPermission();
}

if (permission !== 'granted') {
Copy link
Member

@Feiyang1 Feiyang1 Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't realize Functions was doing that, sorry about that. This was fixed in #2489.

throw errorFactory.create(ErrorCode.NOTIFICATIONS_BLOCKED);
} else if (currentPermission !== 'granted') {
// We must wait for permission to be granted
return null;
}

const swReg = await this.getSWRegistration_();
Expand Down Expand Up @@ -327,10 +329,25 @@ export abstract class BaseController
/**
* Returns the current Notification Permission state.
*/
getNotificationPermission_(): NotificationPermission {
private getNotificationPermission(): NotificationPermission {
return Notification.permission;
}

/**
* Requests notification permission from the user.
*/
private async requestNotificationPermission(): Promise<
NotificationPermission
> {
if (!Notification.requestPermission) {
// Notification.requestPermission() is not available in service workers.
// Return the current permission.
return Notification.permission;
}

return Notification.requestPermission();
}

getTokenDetailsModel(): TokenDetailsModel {
return this.tokenDetailsModel;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/messaging/src/controllers/window-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class WindowController extends BaseController {
* https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission
*/
async requestPermission(): Promise<void> {
if (this.getNotificationPermission_() === 'granted') {
if (Notification.permission === 'granted') {
return;
}

Expand Down
Loading