Skip to content
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

adjust implementation for location watching setup #295

Open
achou11 opened this issue Apr 25, 2024 · 7 comments · May be fixed by #387
Open

adjust implementation for location watching setup #295

achou11 opened this issue Apr 25, 2024 · 7 comments · May be fixed by #387

Comments

@achou11
Copy link
Member

achou11 commented Apr 25, 2024

The implementation of location watching at the time of writing lives in the useLocation hook. As surfaced by the tracks work, one of the limitations in the implementation is that each usage of the hook manages its own internal state and creates a separate subscription for watching position changes. Due to this, there were some parts of the app that one would expect to have the same location information showing at a specific moment but didn't. Each had their own loading state and the time difference in reaching the same state was notable (depending on when initial rendering occurred), making the visual experience feel disjointed.

To solve this issue, the ideal solution is to have one source of truth that manages a single subscription and is responsible for providing the same state to any subscribers. We have something similar for the hooks that use useSyncExternalStore() (e.g. sync state and local discovery state). Ideally we can follow a similar pattern but...

The main trickiness in the case of location updates is that depending on what screen/component is subscribed to updates, we want to be able to limit the update frequency if desired based on some parameters that have changed since the last accepted location update (thinking in terms of React re-renders, basically). This could be desirable for a couple of reasons:

  • a re-render could be costly in resource intensive UIs such as a map.
  • getting frequent updates may not be essential for UX reasons (e.g. the location marker on a map screen may not need to update every time you move meter, whereas the GPS info screen probably should be as up-to-date as possible).

In the immediate, not entirely sure what the best implementation approach would be, but my guess is that it would be easier cleaner to do with useSyncExternalStore() than it would be with Zustand, as I'm not not really sure how Zustand plays with subscriptions handling or internal state in general (might be possible if using a closure works when calling create() 🤔 )

To summarize, the goals are:

  • a single location subscription that's globally available as a singleton
  • a hook that allows distance and time-based "filtering" to limit React re-renders. The filtering is defined per hook usage (i.e. based on consuming component's needs)

I've explored a couple of approaches for implementing what I described above. I haven't fully tested them but at least based on my understanding of React, they seem reasonably sound.

LocationStore

Manages the subscription created by ExpoLocation.watchPositionAsync() and holds the position state that's provided to any subscribers. Although watchPositionAsync() accepts a limited number options for adjusting the update frequency, it is not a goal here to control this at the native subscription level (although it would be ideal). In other words, we configure the subscription to fire off as frequently as the native implementation is able to. Filtering happens at the consumer level, as we'll see below in the hook implementation.

There's an init() method that's needed to initialize the suscription. Ideally this is called near the beginning of app startup, when the necessary permissions are granted.

General implementation
class LocationStore {
  #locationSubscription: LocationSubscription | null = null;
  #subscribers = new Set<() => void>();
  #state: LocationState = {
    location: null,
    error: null,
  };

  /**
   * Initialize position watching. Permissions handling should be done before calling this.
   */
  init = async () => {
    if (this.#locationSubscription) return;

    try {
      this.#locationSubscription = await watchPositionAsync(
        {accuracy: Accuracy.BestForNavigation},
        location => {
          this.#state = {error: null, location};

          for (const s of this.#subscribers) {
            s();
          }
        },
      );
    } catch (err) {
      if (err instanceof Error) {
        this.#state = {location: this.#state.location, error: err};
      }

      for (const s of this.#subscribers) {
        s();
      }
    }
  };

  getSnapshot = (): LocationState => {
    return this.#state;
  };

  subscribe = (cb: () => void) => {
    this.#subscribers.add(cb);

    if (!this.#locationSubscription) {
      this.init().catch(err => {
        console.log(err);
      });
    }

    return () => {
      this.#subscribers.delete(cb);
      if (this.#subscribers.size === 0) {
        this.#locationSubscription?.remove();
        this.#locationSubscription = null;
      }
    };
  };
}

The useLocation() hook (v1)

At first, it wasn't clear to me how to implement this using useSyncExternalStore() because of the filtering requirement. Basically the challenge is making sure we have access to the previous "accepted" state so that the filtering options provided by the hook consumer can be acknowledged. How to implement this was not immediately apparent when first starting, but then I figured that maybe plain old useState() could work well enough! With that, I implemented a version of this refactored hook that uses useState() and useEffect():

General implementation
export function useLocation(
  shouldLocationUpdate: (changes: ParameterChanges) => boolean = () => true,
) {
  const [state, setState] = useState(locationStore.getSnapshot());

  useEffect(() => {
    const unsubscribe = locationStore.subscribe(() => {
      setState(prevState => {
        const newState = locationStore.getSnapshot();

        // 1. If `error` field value is different, apply new state
        if (prevState.error !== newState.error) return newState;

        // 2. If `location` field is non-existent for previous or new state, apply new state
        // Former can technically happen on initialization (although unlikely)
        // Latter should not ever happen (but to make TypeScript happy)
        if (!(prevState.location && newState.location)) return newState;

        const distanceChange = getDistance(
          prevState.location,
          newState.location,
        );

        const timeElapsed =
          newState.location.timestamp - prevState.location.timestamp;

        // 3. Provide the necessary information to let hook consumer decide if new state should be used
        return shouldLocationUpdate({
          distance: distanceChange,
          time: timeElapsed,
        })
          ? newState
          : prevState;
      });
    });

    return () => {
      unsubscribe();
    };
  }, [shouldLocationUpdate]);

  return state;
}

It feels a bit "old school" in terms of managing the hook subscription via useEffect(), but usinguseState() and its functional update parameter made it clear in terms of how to fulfill the filtering requirement. The hook accepts a shouldLocationUpdate parameter, which is a callback that is provided the changes to parameters that are relevant for our filtering requirements (distance and time), and returns a boolean to indicate that the changes fit the filtering requirements of the hook consumer. This provides flexibility for the consumer to decide how the filtering is done based on the context of usage. If omitted, it's equivalent to saying "whenever the location subscription provides a new value, I want to use that new value". If provided, one could decide "I only want to acknowledge new updates if the distance traveled is greater than X meters", as an example.

For the sake of convenience, we can define some custom shouldLocationUpdate functions to handle our specific use cases more easily. For example, we can create a distance-based filter, where usage could look like this:

import {createDistanceFilter, useLocation} from 'hooks/...' // wherever this hook is implemented

// It's useful to create this outside the component because of function stability and how that plays into React render cycle
// Similar to Zustand's limitation around selectors
const atLeastFifteenMetersFilter = createDistanceFilter({
  minimumDistance: 15,
})

function MapScreen() {
  // This state value now only updates when change in distance is at least 15 meters
  const locationState = useLocation(atLeastFifteenMetersFilter)
  
  // ...
}

Once I had this implemented, I had a feeling that this should be feasible to do with useSyncExternalStore(), since what i have implemented here is kind of a rougher version of what it does anyways. Another attempt led me to...

useLocation() (v2)

The implementation using useSyncExternalStore() is pretty close to the v1 implementation, but relies on useRef to create a reference for the previous accepted state value, which is then used for comparison when new updates are emitted by the location subscription. Just need to make that you update it the ref whenever you decide to return the new state value instead of preserving the old one.

General implementation
export function useLocation(
  shouldLocationUpdate: (changes: ParameterChanges) => boolean = () => true,
) {
  const prevState = useRef(locationStore.getSnapshot());

  const getSnapshot = useCallback(() => {
    const newState = locationStore.getSnapshot();

    // 1. If `error` field value is different, apply new state
    if (prevState.current.error !== newState.error) {
      prevState.current = newState;
      return newState;
    }

    // 2. If `location` field is non-existent for previous or new state, apply new state
    // Former can technically happen on initialization (although unlikely)
    // Latter should not ever happen (but to make TypeScript happy)
    if (!(prevState.current.location && newState.location)) {
      prevState.current = newState;
      return newState;
    }

    const distanceChange = getDistance(
      prevState.current.location,
      newState.location,
    );

    const timeElapsed =
      newState.location.timestamp - prevState.current.location.timestamp;

    // 3. Provide the necessary information to let hook consumer decide if new state should be used
    if (
      shouldLocationUpdate({
        distance: distanceChange,
        time: timeElapsed,
      })
    ) {
      prevState.current = newState;
      return newState;
    } else {
      return prevState.current;
    }
  }, [shouldLocationUpdate]);

  return useSyncExternalStore(locationStore.subscribe, getSnapshot);
}

To reiterate, I have NOT tested the above implementations yet but I'm hoping they're closer to what I want our location subscription setup to look like. Being able to have a single subscription and preserve the flexibility for restricted React updates at the component-level is the goal for me. Open to thoughts or suggestions!

@achou11
Copy link
Member Author

achou11 commented Apr 25, 2024

based on some brief testing, there's definitely a bug with the useSyncExternalStore() version of the hook (v2) I've implemented. probably caused by a subtle detail about how that works that I'm missing 🤔 v1 seems promising though...

@achou11
Copy link
Member Author

achou11 commented Apr 29, 2024

I have most of the implementation done for this locally. right now it's closer to v1 than v2, but getting it to v2 is not a must for me (as long as v1 is reasonably sound in terms of react stuff)

@ErikSin
Copy link
Contributor

ErikSin commented May 10, 2024

I wanted to spend some time on this to better understand. So i created a hook based on your suggestions above. The code feels overly complicated, but im not sure if that unavoidable:

Did some testing and it seems to be doing everything i wanted to.

Here is the draft PR

@achou11
Copy link
Member Author

achou11 commented May 10, 2024

@ErikSin thanks for exploring! should've added an update here to point to a branch that I've been working on to implement the new approach and integrate it: https://github.com/digidem/comapeo-mobile/tree/ac/location-refactor

that branch implements both approaches as separate hooks, but currently uses the hook that's closer to approach 1 for the integration in the app. it also does some cleanup and reorganization to account for the new usage.

i have some slight doubts about the useSyncExternalStore implementation as I've described it. If implemented exactly (or close to) the way i described it, there might be a subtle bug related to snapshot method. hard to explain, but basically a consumer of the hook calling getSnapshot() will trigger that call in other components using the hook as well (that's what i noticed, although need to do more testing to confirm). might be an issue with my implementation though, so it's good to have another reference!

@achou11
Copy link
Member Author

achou11 commented May 10, 2024

one thing that's missing from my implementation is the "default" location options we want to filter on. right now, unless you define a filter for where you consume the hook, the hook will fire on each location update (barring a few obvious exceptions). easy to implement, just didn't fully know our minimum requirements and haven't spent the time to actually implement :)

@ErikSin
Copy link
Contributor

ErikSin commented May 11, 2024

Just to clarify, I knew you were working on another branch. I was more so just curious in exploring how i would do based on your approach. The useSyncExternalStore feels like a really odd api to me, so I wanted to practice using it.

But we actually took very different approaches, so curious to connect with you about how you did it.

@ErikSin
Copy link
Contributor

ErikSin commented May 11, 2024

but basically a consumer of the hook calling getSnapshot() will trigger that call in other components using the hook as well

I have no certainty in this statement, but i think it is related to your use of context. Your useuseSycnExternalStore is consuming react state from a context, and then also trying to manage react state separately from the context. It seems like you are using 2 react API's that are designed to do similar things. In my very little exposure to useSycnExternalStore it is typically used in place of a context not with a context.

@achou11 achou11 linked a pull request Jun 4, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants