digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 0 forks source link

chore: refactor location watching #387

Open achou11 opened 1 month ago

achou11 commented 1 month ago

Closes #295

TODO:

ErikSin commented 1 month ago

On review of useLocationProviderStatus, we used it EVERY TIME we used useLocation The idea was that we did not want the user to see a stale location. We were manually checking whether gps was enabled. And if it was not enabled, we would conditionally render the UI to show an undefined value, and if it was enabled we were conditionally rendering the location value. So I just coupled the 2 into one hook. Now when the gps is not enabled, the useLocation hook simply returns location===null, no need to check every time before returning the location value.

In the future we may want to provide the location, even it the gps is not enabled. But I think it would be better to add that as an option to this hook as there would be less maintenance overall.

ErikSin commented 1 month ago

As a more general higher-level suggestion, I don't think the location provider should be setting the location to null based on the provider - I think the consumer component should decide what to display with the location based on that state.

let talk about this tomorrow. My reasoning behind this was that every time we used the location hook we also checked for the provider status. There was never a time we didn't do that. It felt like an unnecessary maintenance burden to have 2 hooks even though they were never being used separately. I thought a good compromise was to create a third hook that combine the 2 hooks. So if we want location to be tied to the provider status (which is currently at all time) we can just use the one hook. But the other 2 hooks are still available if we ever need them separately

ErikSin commented 1 month ago

I resolved a couple comments related to having the 2 subscription in one class. I separated them into 2 implementations of useSyncExternalStore