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

Suggestion - return string instead of number from Geolocation.watchPosition() #328

Closed vegemite4me closed 2 years ago

vegemite4me commented 2 years ago

Hi,

I want to firstly thank you for this great piece of software. It is brilliant, and I am only just starting to learn it. And I can only dream about writing a library like this myself that gets used so widely.

I wanted to make a suggestion about Geolocation.watchPosition() returning a string instead of a number, and I make this suggestion because its current behaviour tripped me up.

Current Behaviour: Geolocation.watchPosition() returns a number, starting from 0. Suggested Behaviour: Geolocation.watchPosition() returns a unique string. (Does not necessarily need to be a UUID.)

I make this suggestion only because I did a falsey check on the result of the call to Geolocation.watchPosition(). I hope the example below shows my mistake. I think returning a string would prevent this happening.

I have fixed my code to handle the fact that watchPosition() returns 0. But I wonder if might help if returned a value that was not falsey?

Robin

e.g.

    const watchId = useRef<number | null>(null);

    useEffect(() => {
        watchId.current = Geolocation.watchPosition(
            (position) => {
                doSomethingWithPosition(position);
            },
            (error) => {
                console.log(error);
            },
            {
                distanceFilter: 1,
            },
        );

        return () => {
            if (watchId.current) {
                Geolocation.clearWatch(watchId.current);   // <- Never called as watchPosition() returned 0.
            }
        }
    }, []);
Agontuk commented 2 years ago

You should check for watchId.current !== null. It's implemented this way to be compatible with web api which returns number as well.

vegemite4me commented 2 years ago

A good reason to leave it as a number. Thanks @Agontuk

Agontuk commented 2 years ago

NP, closing this issue then.