Skip to content

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Jan 27, 2020

Why

We want to let people set badge counter on demand.

How

  • on Android we use ShortcutBadger
  • on iOS we use applicationIconBadgeNumber
  • on web we use badgin

API

getBadgeCountAsync(): Promise<number>;
setBadgeCountAsync(badgeCount: number, undefined | {
  web?: badgin.Options
}): Promise<boolean>;

Test Plan

Tested the module on iOS 13 (worked ✅), Android 7.1 (Samsung, default launcher, 👎), Android 7.1 (Samsung, ADW Launcher, ✅), Android 9 (Nokia, default launcher, 👎), web (worked ✅). In order to be able to work around unsupported launchers I added a simple infrastructure inside NewNotifications.js test suite to skip tests on unsupported devices.

@tsapeta tsapeta linked an issue Feb 5, 2020 that may be closed by this pull request
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-badges branch 4 times, most recently from 239d2b0 to a3029b6 Compare February 18, 2020 09:15
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-badgesmodule branch from 315287c to eda53b8 Compare February 18, 2020 10:19
@sjchmiela sjchmiela changed the base branch from @sjchmiela/expo-notifications-badges to master February 18, 2020 10:20
@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-badgesmodule branch from eda53b8 to da8f285 Compare February 18, 2020 11:06
@sjchmiela sjchmiela marked this pull request as ready for review February 18, 2020 11:08
@sjchmiela sjchmiela requested a review from mczernek as a code owner February 18, 2020 11:08
@sjchmiela sjchmiela requested a review from esamelson February 18, 2020 11:08
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

My only comment here is instead of rejecting/throwing when we're unable to set the badge count, I wonder if we should just resolve the promise with false (and with true when we are successful). In general I think rejecting should be reserved for actual errors and exceptions, and here we expect a fair amount of devices in production to have this failure -- which developers have no control over. Also, there's not really any alternative to this behavior so I think most developers would just catch the error and then do nothing. (or have an unhandled promise rejection) --> resolving with false is better and people can handle it if they want to.

This would also take care of the weird test case -- we can just expect either true or false and the test will only fail if an actual exception occurs.

@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-badgesmodule branch from da8f285 to 6d9afc1 Compare February 26, 2020 14:06
@sjchmiela
Copy link
Contributor Author

Following your suggestion I have changed the setBadgeCountAsync signature to return Promise<boolean>. The value returned is:

  • on Android: whether ShortcutBadger reports it succeeded
  • on iOS: whether current notification settings allow access to icon badge number

    In iOS 8.0 and later, your application must register for user notifications using -[UIApplication registerUserNotificationSettings:] before being able to set the icon badge.

  • on Web: always true, since badgin itself implements some fallbacks, one of which is changing window.title which rarely fails

@sjchmiela sjchmiela force-pushed the @sjchmiela/expo-notifications-badgesmodule branch from 6d9afc1 to b467006 Compare March 5, 2020 08:25
@sjchmiela sjchmiela merged commit b286d3e into master Mar 5, 2020
@sjchmiela sjchmiela deleted the @sjchmiela/expo-notifications-badgesmodule branch March 5, 2020 08:25
@mikeRChambers610
Copy link

mikeRChambers610 commented Jun 17, 2021

This does not work on Android, but returns a promise with "true":

Notifications.setBadgeCountAsync(0);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract Notifications to unimodule
3 participants