iOS: Geolocation: Allow skipping of permission prompts #15096

rigdern · 2017-07-19T03:14:35Z

This change enables developers to tell the Geolocation module to skip its permissions requests so the app can manage the permissions requests on its own. React Native Android's Geolocation module already requires apps to request permissions on their own.

Currently, the Geolocation module automatically makes permissions requests for you based on what you've specified in your property list file. However, the property list file doesn't always represent what permissions the app wants right now. For example, suppose an app sometimes wants location when "in use" and sometimes wants location "always" depending on what app features the user has engaged with. The Geolocation API will request the "always" location permission even if that's not what the app wants for the current scenario.

This change enables developers to tell the Geolocation module to skip permission requests so that the app can explicitly ask for the appropriate permissions. By default, Geolocation will still ask for permissions so this is not a breaking change.

This change adds a method to Geolocation that is not supported by the web spec for geolocation (setRNConfiguration). This method includes RN in the name to minimize the chances that the web spec will introduce an API with the same name.

Test plan (required)

Verified that Geolocation requests permissions by default and when skipPermissionRequests is false. Verified the permission requests are skipped when skipPermissionRequests is true. Also, my team is using this change in our app.

Adam Comella
Microsoft Corp.

chirag04 · 2017-08-09T20:00:19Z

Libraries/Geolocation/RCTLocationObserver.m

@@ -135,7 +149,9 @@ - (dispatch_queue_t)methodQueue

 - (void)beginLocationUpdatesWithDesiredAccuracy:(CLLocationAccuracy)desiredAccuracy distanceFilter:(CLLocationDistance)distanceFilter
 {
-  [self requestAuthorization];
+  if (!_locationConfiguration.skipPermissionRequests) {


wondering if we can just check that permissions are already granted instead of a skip flag

@chirag04 That's an interesting suggestion. A downside to that design is it'll open the door to a tricky to catch app bug. Suppose the app wants to request permissions on its own but fails to. With the current implementation, the app will see an exception due to not having the proper permissions. With your suggestion, RN will request permissions on behalf of the app making it difficult for the developer to realize there's a bug in their app.

javache · 2017-08-10T12:40:16Z

Libraries/Geolocation/Geolocation.js

+    *
+    */
+  setRNConfiguration: function(
+    geo_config: GeoConfiguration


Use camelCase. In this case, just name the argument config

javache · 2017-08-10T12:40:52Z

Libraries/Geolocation/RCTLocationObserver.m

@@ -160,6 +176,11 @@ - (void)timeout:(NSTimer *)timer

 #pragma mark - Public API

+RCT_EXPORT_METHOD(setRNConfiguration:(RCTLocationConfiguration)config)


Why the RN in the name? Can this be just setConfiguration:

@javache "This method includes RN in the name to minimize the chances that the web spec will introduce an API with the same name."

Sorry, missed that in your original description. I think it's a good idea to avoid potential conflicts with the web spec. We can name the native module however we want though, so let's call the native method setConfiguration and the API on geolocation setReactConfiguration?

javache · 2017-08-10T12:41:25Z

ReactAndroid/src/main/java/com/facebook/react/modules/location/LocationModule.java

@@ -104,6 +104,11 @@ private static LocationOptions fromReactMap(ReadableMap map) {
    }
  }

+  @ReactMethod


Remove this completely and instead check on the JS side if the method exists before calling it.

hramos · 2017-08-15T17:00:28Z

I've taken a look at the current status of this PR, and based on the comments left by other reviewers as well as the conflicts, I'm marking it as "needs-revision".

Currently, the Geolocation module automatically makes permissions requests for you based on what you've specified in your property list file. However, the property list file doesn't always represent what permissions the app wants right now. For example, suppose an app sometimes wants location when "in use" and sometimes wants location "always" depending on what app features the user has engaged with. The Geolocation API will always request the "always" location permission even if that's not what the app wants for the current scenario. This change enables developers to tell the Geolocation module to skip permission requests so that the app can explicitly ask for the appropriate permissions. By default, Geolocation will still ask for permissions so this is not a breaking change. This change adds a method to Geolocation that is not supported by the web spec for geolocation (`setRNConfiguration`) This API includes `RN` in the name to minimize the chances that the web spec will ever introduce an API with the same name. **Test plan (required)** Verified that Geolocation requests permissions by default and when `skipPermissionRequests` is `false`. Verified the permission requests are skipped when `skipPermissionRequests` is `true`. Also, my team is using this change in our app. Adam Comella Microsoft Corp.

rigdern · 2017-08-16T23:37:59Z

@hramos thanks for the ping.

@javache thanks for reviewing this. I pushed an update which addresses your comments. As described in my other comment, I left the method setRNConfiguration with the RN prefix. I justified why I used the prefix but if you'd prefer, I can remove the prefix. Let me know.

rigdern · 2017-08-27T15:59:20Z

PR #15067 also solves this problem. We should figure out which PR to take. See my comment (#15067 (comment)) for a comparison of these PRs.

facebook-github-bot · 2017-08-29T09:53:07Z

@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ngandhy · 2018-01-08T22:15:56Z

@rigdern - when I add the line "navigator.geolocation.setRNConfiguration({'skipPermissionRequests':true});" to my code the location times out every time.

The second I remove the line of code, everything works again. Any tips / advice?

ngandhy · 2018-01-08T22:32:33Z

@rigdern @javache @hramos - Found the issue. _locationManager is never being set if you skip the permission request using skipPermissionsRequest:true. This is because _locationManager is being set INSIDE the authorization method.

Adding the following inside beginLocationUpdatesWithDesiredAccuracy fixes the issue.

if (!_locationManager) {
_locationManager = [CLLocationManager new];
_locationManager.delegate = self;
}

rigdern · 2018-01-08T22:43:13Z

@ngandhy That looks like a bug. Good find! Do you want to send a PR?

Adam Comella
Microsoft Corp.

ngandhy · 2018-01-08T23:34:32Z

kk, I'll give it a try :)

Also, I've seen your features, fixes, and all the other stuff you're doing for the community and just wanted to say THANKS! I'm sure you guys don't hear it as much as you should

rigdern · 2018-01-13T00:00:18Z

Also, I've seen your features, fixes, and all the other stuff you're doing for the community and just wanted to say THANKS! I'm sure you guys don't hear it as much as you should

@ngandhy, thanks, I'm glad you've found our work to be valuable :)

Summary: This fixes #17486 make sure locationManager is being set before continuing. Used if !locationManager vs else on previous statement as we should NEVER enter this code without _locationManager set. Also the else version might experience issues if someone touches the auth code and doesn't check this case, so seems more "long term stable".  This fixes #17486 1) Have a working geolocation demo 2) Add navigator.geolocation.setRNConfiguration({'skipPermissionRequests':true}); to your code. I added it to the constructor or componentWillMount for the app. 3) Observe that geolocation no longer works (times out) 4) Apply patch 5) Observe that geolocation works again re #15096  [IOS] [BUGFIX] [GeoLocation] - Fix skipPermissionRequests by setting _locationManager Closes #17487 Differential Revision: D6718389 Pulled By: hramos fbshipit-source-id: 08c1c9306b4d87cc40acdaa1550bb6df8345db02

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jul 19, 2017

facebook deleted a comment from pull-bot Jul 19, 2017

chirag04 reviewed Aug 9, 2017

View reviewed changes

javache reviewed Aug 10, 2017

View reviewed changes

hramos added GH Review: needs-revision and removed GH Review: review-needed labels Aug 15, 2017

Adam Comella added 2 commits August 16, 2017 15:35

PR feedback

e6a02af

rigdern force-pushed the rigdern/skip-location-prompts branch from aabaecc to e6a02af Compare August 16, 2017 23:11

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Aug 16, 2017

Remove setRNConfiguration stub on Android

c3289ac

ngandhy mentioned this pull request Aug 19, 2017

getCurrentPosition Prompts the "Always Location" permission instead of the "On Use" permission #11822

Closed

rigdern mentioned this pull request Aug 27, 2017

[Geolocation] Provide option to request specific location permissions #15067

Closed

javache added 2 commits August 29, 2017 10:51

Update native API

d79cda6

Update Geolocation.js

6a658f7

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Aug 29, 2017

facebook-github-bot closed this in 6b7f10d Aug 29, 2017

rigdern deleted the rigdern/skip-location-prompts branch August 30, 2017 07:16

ngandhy mentioned this pull request Jan 8, 2018

Geolocation times out when using "skipPermissionRequests" #17486

Closed

ngandhy mentioned this pull request Jan 8, 2018

re #17486 - set _locationManager if auth skipped #17487

Closed

ttahmouch mentioned this pull request Mar 9, 2018

Automatic requests using PermissionsAndroid should allow the ability to be skipped. #18306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS: Geolocation: Allow skipping of permission prompts #15096

iOS: Geolocation: Allow skipping of permission prompts #15096

rigdern commented Jul 19, 2017

chirag04 Aug 9, 2017 •

edited

Loading

rigdern Aug 9, 2017

javache Aug 10, 2017

javache Aug 10, 2017

rigdern Aug 10, 2017

javache Aug 25, 2017

javache Aug 10, 2017

hramos commented Aug 15, 2017

rigdern commented Aug 16, 2017

rigdern commented Aug 27, 2017

facebook-github-bot commented Aug 29, 2017

ngandhy commented Jan 8, 2018

ngandhy commented Jan 8, 2018

rigdern commented Jan 8, 2018

ngandhy commented Jan 8, 2018

rigdern commented Jan 13, 2018

		@@ -160,6 +176,11 @@ - (void)timeout:(NSTimer *)timer

		#pragma mark - Public API

		RCT_EXPORT_METHOD(setRNConfiguration:(RCTLocationConfiguration)config)

iOS: Geolocation: Allow skipping of permission prompts #15096

iOS: Geolocation: Allow skipping of permission prompts #15096

Conversation

rigdern commented Jul 19, 2017

chirag04 Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

rigdern Aug 9, 2017

Choose a reason for hiding this comment

javache Aug 10, 2017

Choose a reason for hiding this comment

javache Aug 10, 2017

Choose a reason for hiding this comment

rigdern Aug 10, 2017

Choose a reason for hiding this comment

javache Aug 25, 2017

Choose a reason for hiding this comment

javache Aug 10, 2017

Choose a reason for hiding this comment

hramos commented Aug 15, 2017

rigdern commented Aug 16, 2017

rigdern commented Aug 27, 2017

facebook-github-bot commented Aug 29, 2017

ngandhy commented Jan 8, 2018

ngandhy commented Jan 8, 2018

rigdern commented Jan 8, 2018

ngandhy commented Jan 8, 2018

rigdern commented Jan 13, 2018

chirag04 Aug 9, 2017 •

edited

Loading