This differential uses native API to request notifications permissions in JavaScript on Android 13 devices.
Details
Details
Build and install the app on Android 13 device. Ensure the following:
- After you log in the OS prompts you to grant notifications permission. If you see this prompt before you log in, it is an error.
- If you don't allow this permission you are never prompted again (this is what happens on iOS).
- If you don't allow this permission ensure you don't see "Unable to initialize notifs" alert that is present on older Androids.
Build the app on older Android device. Ensure no change in behavior occurred.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Just a question
| native/push/push-handler.react.js | ||
|---|---|---|
| 330 ↗ | (On Diff #23925) | Can you explain why it's necessary to check the permission if we already have it? |
| native/push/push-handler.react.js | ||
|---|---|---|
| 330 ↗ | (On Diff #23925) | This if does not check whether we have permission but whether we can request it. It is basically more idiomatic way to check whether we are on Android 13 and we should try requesting permission from user since now they are not granted by default. On older androids canRequestNotificationsPermissionFromUser will resolve to hasPermission if a JS developer accidentally calls this method, so this check does not save us from an error, but just eliminates unnecessary call. However I have an intuition that what you actually asked we rather why do we do this: |
| native/push/push-handler.react.js | ||
|---|---|---|
| 326–328 ↗ | (On Diff #23925) | I don't understand the concern here. Both of those promises are awaited before we use their results to decide whether to issue notifications permission request. |
| native/push/push-handler.react.js | ||
|---|---|---|
| 326–328 ↗ | (On Diff #23925) | I think the suggestion might be to use await Promise.all([...]) so they can both be "kicked off" simultaneously? |
Comment Actions
Please make the requested changes before landing
| native/push/push-handler.react.js | ||
|---|---|---|
| 326–328 ↗ | (On Diff #24082) | Yes, @atul is right. You are running these promises in sequence instead of in parallel. Please use Promise.all to run them in parallel, and please always try to reduce the number of await keywords in an async function |
| 333 ↗ | (On Diff #24082) | We don't need this anymore... we know hasPermission is false because of the condition on line 330 |
| native/push/push-handler.react.js | ||
|---|---|---|
| 332 ↗ | (On Diff #24082) | Following discussion in D7106 I took a closer look at this code. The risk I highlighted there (which I remain concerned about) is the possibility of a second ensureAndroidPushNotifsEnabled call before the first completes, which could potentially result from eg. backgrounding / foregrounding the app quickly, or eg. a foreground timed at the same time as a logic. I think @marcin's proposal in D7106 there was reasonable: instead of throwing, have the second call return a special value that tells the caller to basically "do nothing". This would avoid any weird behavior from the second call, and then presumably the first call would handle the intention of the second call. My proposal has the same benefits, but arguably is a little cleaner. We can guarantee on the JS side that we only call requestNotificationsPermission once at a time: function useRequestNotificationsPermissionFunc(): () => Promise<boolean> { const promiseRef = React.useRef<?Promise<boolean>>(); return React.useCallback(() => { if (!promiseRef.current) { promiseRef.current = (async () => { const notifPermission = await CommAndroidNotifications.requestNotificationsPermission(); promiseRef.current = undefined; return notifPermission; })(); } return promiseRef.current; }, []); } |
| native/push/push-handler.react.js | ||
|---|---|---|
| 332 ↗ | (On Diff #24082) | s/logic/login, sorry |
Comment Actions
- Ensure there is only one promise calling Java notifications permission code at a time.
- Use Promise.all
Comment Actions
Sleep to ensure logged out model disappears before the prompt launches. Rebase before landing