-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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.
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
[PermissionsAndroid] Handle "Never Ask Again" in permissions and add requestMultiplePermissions #10221
Conversation
…permissions request at the same time
By analyzing the blame information on this pull request, we identified @foghina and @andreicoman11 to be potential reviewers. |
Also note - this is a breaking change in the API. It changes the result of requestPermission to a string (with 3 possible results) as opposed to a boolean |
@cmcewen updated the pull request - view changes |
I think better not to have a breaking change and instead introduce a new method. We can add deprecated warning to current methods and eventually remove them. What do you think of following the We could have The shape of the object passed could be something like |
@alexHlebnikov still seeing crashes as well. take a look at #10351 |
Summary: We're seeing a lot of crashes from `PermissionsModule` not being able to access the current activity, mentioned in #10009 and here: #9310 (comment) As far as I can tell, there is no way to ensure the Activity exists since the `ReactContext` holds a `WeakReference` to the current Activity and it appears that the lifecycle calls are happening in the right order (so not the same as #9310). This will at least allow people to catch the error in JS and update the UI or try again as opposed to crashing the app. I'm working on some bigger changes in #10221 but this is a smaller change and important to get fixed I think. Closes #10351 Differential Revision: D4010242 fbshipit-source-id: 7a76973bb2b3e45817d4283917740c89a10ec0b0
@AndrewJack was planning to circle back on this - will need to incorporate changes from #10296 too. As for the generic Permissions API, I've thought about it a bit but I'm not sure this is something that can be abstracted across platforms - iOS and Android handle things fairly differently and with iOS 10 we now have to include the If we don't adopt that though and remove the breaking change, what do you think the new method should be? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow types will help if we do make a breaking change
@@ -128,6 +135,15 @@ class PermissionsAndroid { | |||
} | |||
return Permissions.requestPermission(permission); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change flow return type from Promise<boolean>
to Promise<string>
Will at least warn people using flow that this api has changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although because this is javascript, if (granted) {
will just evaluate as true if granted is a non empty string. So we won't get a a flow warning.
* returns an object with the permissions as keys and strings as values | ||
* indicating whether the user allowed or denied the request | ||
*/ | ||
requestMultiplePermissions(permissions: Array<string>) : Promise<Object> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a stricter flow return type:
{ [permission: string]: string }
Summary: We're seeing a lot of crashes from `PermissionsModule` not being able to access the current activity, mentioned in #10009 and here: #9310 (comment) As far as I can tell, there is no way to ensure the Activity exists since the `ReactContext` holds a `WeakReference` to the current Activity and it appears that the lifecycle calls are happening in the right order (so not the same as #9310). This will at least allow people to catch the error in JS and update the UI or try again as opposed to crashing the app. I'm working on some bigger changes in #10221 but this is a smaller change and important to get fixed I think. Closes #10351 Differential Revision: D4010242 fbshipit-source-id: 7a76973bb2b3e45817d4283917740c89a10ec0b0
It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @foghina as a potential reviewer. Could you take a look please or cc someone with more context? |
It's just to have a familiar API. It's okay to diverge a bit to fit specific platform usage.
We make breaking changes all the time, but most of the time we deprecate the old APIs for a few releases before removing them entirely.
I think this PR is a good time to do it since otherwise we don't have a good migration plan for existing users. Even though a minority of people are using it, we will break their code without any warning. So I think we should avoid it. Also migrating to the new API is jus renaming few methods and such, so I hope it's not a large task.
I think just |
@cmcewen are you still planning to ship this? |
@satya164 revisiting this with a few updates - sorry for taking so long. I did some more reading on the web permissions API and though I agree it would be really nice to have a familiar API that has cross-platform potential, the reality of RN right now doesn't seem like it would be the right fit. There are only 4 permissions in the web spec right now (https://www.w3.org/TR/permissions/#permission-registry): By introducing a generic Permissions API as a new module, it would seem to lead that RN will eventually support these 4 permissions on both platforms + other web permissions as they are released. To me it seems highly unlikely that will happen. It also seems strange that using the Permissions API will require importing a list of Android specific permissions through the PermissionsAndroid module. As for the breaking change, I understand usually it is frowned upon, but anyone using this API will already be used to jumping through a few hoops. Otherwise it would require either creating this generic API to have a new method, or adding a new method to PermissionsAndroid with a name other than |
@cmcewen Probably I didn't explain properly, but my intention is not to provide a cross-platform Basically just And instead of returning a boolean, return Renaming the methods is straightforward and will also allow us to smoothly deprecate the old methods. |
@satya164 ah okay that makes more sense - I've updated to use that syntax. I return The spec says: Never ask again is important to know because the user won't be shown any system prompt at all, so they should be directed to the system settings where they can enable the permission if it's really important for the app's functionality |
@cmcewen sounds good |
Can you also add warnings when trying to use deprecated methods? |
* Returns a promise resolving to a boolean value as to whether the specified | ||
* permissions has been granted | ||
*/ | ||
query(permission: string) : Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to change this to check
since it doesn't actually return the permission status (which is what I thought at first)
* (https://developer.android.com/training/permissions/requesting.html#explain) | ||
* and then shows the system permission dialog | ||
*/ | ||
async request(permission: string, rationale?: Rationale) : Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add type PermissionStatus = 'granted' | 'denied' | 'never_ask_again'
at the top and change Promise<string>
to Promise<PermissionStatus>
* returns an object with the permissions as keys and strings as values | ||
* indicating whether the user allowed or denied the request | ||
*/ | ||
requestMultiple(permissions: Array<string>) : Promise<{[permission: string]: string}> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Promise<{[permission: string]: PermissionStatus}>
Looks good to me, just few minor things and I'll merge! Thanks a lot :D |
@facebook-github-bot shipit |
…sions Summary: In order to get featured in the Google Play Store, we had to handle a few specific cases with permissions based on feedback from the editorial team. First, which was previously possible with this permissions module was bumping the sdk to version 23. The second is requesting multiple permissions at one time. In order for the camera + upload to work, we needed to request both camera permissions + media storage in one flow. The last is handling the case where the user checks the "Never Ask Again" box. This will only appear after a user denies a permission once and is then prompted again. The logic for handling this case is taken from here: http://stackoverflow.com/questions/31928868/how-do-we-distinguish-never-asked-from-stop-asking-in-android-ms-runtime-permis/35495372#35495372 We were also seeing a few crashes similar to facebook#10009 due to `onRequestPermissionsResult` being called before `onResume` (http://stackoverflow.com/questions/35205643/why-is-onresume-called-after-onrequestpermissionsresult), so I delaye Closes facebook#10221 Differential Revision: D4232551 fbshipit-source-id: fee698d1c48a2d86623cb87996f3d17f4c10a62e
…sions Summary: In order to get featured in the Google Play Store, we had to handle a few specific cases with permissions based on feedback from the editorial team. First, which was previously possible with this permissions module was bumping the sdk to version 23. The second is requesting multiple permissions at one time. In order for the camera + upload to work, we needed to request both camera permissions + media storage in one flow. The last is handling the case where the user checks the "Never Ask Again" box. This will only appear after a user denies a permission once and is then prompted again. The logic for handling this case is taken from here: http://stackoverflow.com/questions/31928868/how-do-we-distinguish-never-asked-from-stop-asking-in-android-ms-runtime-permis/35495372#35495372 We were also seeing a few crashes similar to #10009 due to `onRequestPermissionsResult` being called before `onResume` (http://stackoverflow.com/questions/35205643/why-is-onresume-called-after-onrequestpermissionsresult), so I delaye Closes facebook/react-native#10221 Differential Revision: D4232551 fbshipit-source-id: fee698d1c48a2d86623cb87996f3d17f4c10a62e
In order to get featured in the Google Play Store, we had to handle a few specific cases with permissions based on feedback from the editorial team.
First, which was previously possible with this permissions module was bumping the sdk to version 23.
The second is requesting multiple permissions at one time. In order for the camera + upload to work, we needed to request both camera permissions + media storage in one flow.
The last is handling the case where the user checks the "Never Ask Again" box. This will only appear after a user denies a permission once and is then prompted again. The logic for handling this case is taken from here: http://stackoverflow.com/questions/31928868/how-do-we-distinguish-never-asked-from-stop-asking-in-android-ms-runtime-permis/35495372#35495372
We were also seeing a few crashes similar to #10009 due to
onRequestPermissionsResult
being called beforeonResume
(http://stackoverflow.com/questions/35205643/why-is-onresume-called-after-onrequestpermissionsresult), so I delayed the handling using a callback.