Agontuk / react-native-geolocation-service

React native geolocation service for iOS and android
https://www.npmjs.com/package/react-native-geolocation-service
MIT License
1.6k stars 291 forks source link

Geolocation.getCurrentPosition falsely fires errorCallback when setting timeout on iOS #291

Closed jdrorrer closed 2 years ago

jdrorrer commented 3 years ago

Actual: When setting a timeout manually for Geolocation.getCurrentPosition on iOS, the errorCallback is called at the end of the timeout no matter what. This makes it difficult to properly handle errors when retrieving a user's location and also causes error monitoring software to get blown up with false negatives.

In our case specifically, we show a list of store locations nearby on app init - if the app could not find their location, we would clear out the user's data and show them an alert stating "Could not find location" since the app is not useable without location data. This would render the app unusable even with the user's location data.

However, this package does cache their location - because the successCallback actually was called the first time around, if the user closes the app then re-opens it, it will have their location stored and show their relevant location-based data.

Expected: When a user's location is found I would expect the successCallback to be called but not the errorCallback. The errorCallback should only be called if the timeout expires without successCallback being called.

jdrorrer commented 3 years ago

To fix this, I updated the RNFusedLocation.swift file to add a guard that prevents firing errorCallback if successCallback was called before timeout expires. I used the patch-package library with the following diff:

diff --git a/node_modules/react-native-geolocation-service/ios/RNFusedLocation.swift b/node_modules/react-native-geolocation-service/ios/RNFusedLocation.swift
index 5538e1e..5fcab37 100644
--- a/node_modules/react-native-geolocation-service/ios/RNFusedLocation.swift
+++ b/node_modules/react-native-geolocation-service/ios/RNFusedLocation.swift
@@ -24,6 +24,7 @@ class RNFusedLocation: RCTEventEmitter {
   private var resolveAuthorizationStatus: RCTPromiseResolveBlock? = nil
   private var successCallback: RCTResponseSenderBlock? = nil
   private var errorCallback: RCTResponseSenderBlock? = nil
+  private var success: Bool = false

   override init() {
     super.init()
@@ -96,6 +97,7 @@ class RNFusedLocation: RCTEventEmitter {
       if elapsedTime < maximumAge {
         // Return cached location
         successCallback([lastLocation])
+        success = true
         return
       }
     }
@@ -159,6 +161,8 @@ class RNFusedLocation: RCTEventEmitter {
     let errorCallback = data["errorCallback"] as! RCTResponseSenderBlock
     let manager = data["manager"] as! CLLocationManager

+    guard success == false else { return }
+
     manager.stopUpdatingLocation()
     manager.delegate = nil
     errorCallback([generateErrorResponse(code: LocationError.TIMEOUT.rawValue)])
@@ -324,6 +328,7 @@ extension RNFusedLocation: CLLocationManagerDelegate {

     lastLocation = locationData
     successCallback!([locationData])
+    success = true

     // Cleanup
     manager.stopUpdatingLocation()
@@ -331,6 +336,7 @@ extension RNFusedLocation: CLLocationManagerDelegate {
     timeoutTimer?.invalidate()
     successCallback = nil
     errorCallback = nil
+    success = false
   }

   func locationManager(_ manager: CLLocationManager, didFailWithError error: Error) {
@@ -372,5 +378,6 @@ extension RNFusedLocation: CLLocationManagerDelegate {
     timeoutTimer?.invalidate()
     successCallback = nil
     errorCallback = nil
+    success = false
   }
 }
Agontuk commented 2 years ago

Please try the latest version & see if it solves the issue.

jdrorrer commented 2 years ago

@Agontuk this appears to be fixed in the latest version (5.3.0-beta.4 as of today). When I wrote the patch above, I was on version 5.3.0-beta.1 - just a heads up for anyone else coming across this issue.

Thank you @Agontuk!