I understand, but I implemented it this way with purpose. Perhaps in future we might use requestPermission for another type of runtime permission. In this case it will be easy to modify this callback by adding new if statement. If I add early exit now, we might need to remove it in future.

If it is a common practice to reject then I will stick to the conversion. I was thinking about this actually how to handle already ongoing request. I was inspired by solution adopted by Notifee library: https://github.com/invertase/notifee/pull/523/commits/273ff30f99c0ee17d30c62da18ed0cce93fd7731. After reading through their code I understand they substitute already existing promise with new one. On the other hand, they don't use any synchronization primitives - they just keep raw reference to the promise. It just doesn't sound right to me so I am not strongly inclined to consider their implementation as a gold standard.

This code can be any number, it's used to ensure that onRequestPermissionsResult() is triggered as a response to our request.
(multiple permission requests could be sent simultaneously, but there's only one callback)

Yes, that was discussed offline and this rejection addresses my comment above: https://phab.comm.dev/D7106?id=23924#inline-46661

This rejection doesn't have to be explicitly handled on JS side, it's just a kind of 'invariant' note that this was accidentally called twice.

Got it – would be good to explain this in a code comment

Ah, so you're saying it's possible for this line to be triggered as a result of JS programmer error? Can you clarify what sort of error the JS programmer would have to make? Eg. is there an expectation that the JS programmer will not call requestNotificationsPermission before the last requestNotificationsPermission is complete?

I will add a comment. And yes - @bartek explanation is exactly the reason this request code is hardcoded. Additionally I chose the number 11111 since the same number was used by Notifee library in their PR that migrates to Android 13: https://github.com/invertase/notifee/blob/main/android/src/main/java/app/notifee/core/Notifee.java#L49

Why is it confusing? I want to use the same functionality both in this Java class and in JS via exposed ReactMethod. I wanted to avoid repeating code that calls NotificationManagerCompat api/check for SDK_INT.

I think this is exactly the case @bartek wanted to handle. We expect this method not to be called twice in JS before the first completes. If JS programmer make this error they will see an exception that will force them to fix it. However if it is not acceptable to enforce this on the JS side I can work on another approach to handle this case.

Sure - I will change it.

My understanding is that these are two different methods, just with the same method name. Java lets you do this because it allows method name overloading. I think it's confusing because these two methods are doing different things.

I'm not sure what you mean when you say "I want to use the same functionality". I don't understand how renaming your method to be more clear affects your ability to use functionality, or causes code repetition. Can you clarify what code you think will need to be repeated if you rename the methods on line 189 and 194?

Are we really prepared to guarantee that on the JS side? I suspect that this API will be triggered by a React state transition (eg. in a React.useEffect or in componentDidUpdate), which means that the JS side has to cache the promise in order to guarantee that it will only be executed once.

We can do this in JS, but there are two downsides in comparison to doing it in Java:

1. This requirement (API can only be called once at a time) is not very discoverable, and might not come up in testing. As a result, the JS programmer could make the mistake of not considering it.
2. If we do it in Java, we make it impossible for the API to be used incorrectly. We need to handle this "synchronization" somewhere, so it seems better to handle it in such a way that does not introduce the possibility of programmer error.

On the other hand, this synchronization might be harder to do in Java. I don't have context on why we opted not to do it in Java – can you please share if there are any reasons?

I don't understand why we're wasting so much review cycle time on such a minor thing. I'm starting to wonder if it'd be better to revert my suggestion just to avoid all that fuss.

Seriously, it's just a good practice to reject subsequent promises if one is still in progress. That's all. This is a way to prevent accidentally opening two native permission/image-picker/whatever interfaces at once. I've never seen that anybody ever cared about this that much, moreover I haven't seen anybody explicitly handling this in JS. It's obvious that you want to display it once (= call it once at a time = don't call it again until awaited). So in a typical scenario, this will never throw.

It is less about synchronization but more about what should we do if we issue a second call to this method before promise from the first call completes. My first idea was to atomically substitute the first promise with the second one. However this would leave this the first promise unresolved. Therefore @bartek requested to change this behaviour by rejecting the second promise. Alternatively we can just resolve the second promise to the value returned by hasPermission method. Even if the second promise resolves to invalid value (since the interaction between user and notifications permission prompt is still ongoing and may change hasPermission value) the the first will resolve to the correct value and will fix the state.

cc @ashoat , @bartek what do you think?

I wanted not to repeat those fragments of code:

if (android.os.Build.VERSION.SDK_INT >=
       android.os.Build.VERSION_CODES.TIRAMISU) {
     // Application has to explicitly request notifications permission from
     // user since Android 13. Older versions grant them by default. Details:
     // https://developer.android.com/develop/ui/views/notifications/notification-permission
     return true;
   }
   return false;

and

return NotificationManagerCompat.from(getReactApplicationContext())
        .areNotificationsEnabled();

I needed them both in Java code and in JS-exported methods, so I decided to extract them to private Java methods. I used overloading to name those methods since it was natural for me.
I will add Internal suffixes to names of those methods.

It is less about synchronization but more about what should we do if we issue a second call to this method before promise from the first call completes. My first idea was to atomically substitute the first promise with the second one. However this would leave this the first promise unresolved.

Exactly!

Alternatively we can just resolve the second promise to the value returned by hasPermission method. Even if the second promise resolves to invalid value (since the interaction between user and notifications permission prompt is still ongoing and may change hasPermission value) the the first will resolve to the correct value and will fix the state.

Order of resolving them is not guaranteed so we could have a race condition here, I'd just leave it as is

After gathering the discussion above my statement on this is that:

1. I would follow @bartek approach since as he stated it is a common practice he encountered in open source projects.
2. If there is no acceptance to this approach I will make this method resolve not to two but to three values: false, true, null. The promise resolved to null will be interpreted in JS code that the request is already in progress and another promise will shortly resolve to some meaningful value we can react to (by querying for device token or setting it to null). It will probably be an early return in JS. Alternatively to resolving to ?boolean we can resolve to string constants such as GRANTED, DENIED and REQUEST_ALREADY_IN_PROGRESS.