Whoops, we called that API twice

In one of my codebases at work we have a custom React hook for rendering the result of a promise:

export function usePromise<T>(
  p: Promise<T>,
): [null | T, Error | null, boolean, (p: Promise<T>) => void] {
  const [promise, setPromise] = useState<Promise<T>>(p);
  const [state, setState] = useState<T | null>(null);
  const [error, setError] = useState<Error | null>(null);
  const [loading, setLoading] = useState(true);
  useEffect(() => {
    let cancelled = false;
    promise
      .then((result) => !cancelled && setState(result))
      .catch((error) => !cancelled && setError(error))
      .finally(() => !cancelled && setLoading(false));
    return () => {
      cancelled = true;
    };
  }, [promise]);
  return [state, error, loading, setPromise];
}

At first blush this may seem fine. This function takes a promise object, sets it on state, and then fires a useEffect to register a callback that updates component state with the result of the promise.

Here’s what the call site looked like:

  const [subscription, subscriptionError] = usePromise(
    userStore.getActiveUserSubscription(),
  );

But there’s a sneaky bug here. When reviewing the tests for this component I discovered that the API for getActiveUserSubscription was being called twice, when I’d expect it only to be called once, on mount.

The Render Function

The issue is that the usePromise function expects a promise to be passed in directly, and the call site was constructing that promise directly in the render function of the component, instead of in a useEffect. The test handler was calling the render function on this component, and the call to getActiveUserSubscription was being newly run on each render, yikes! If you’re not familiar with why this is a problem, remember:

The render function of a component should only ever generate view objects when run, not trigger side-effects. All side-effect code should be done from a useEffect hook, where the re-running behavior is controllable. The programmer doesn’t control the re-running behavior of the render function.

A Better Version

Here’s an updated version of usePromise with better ergonomics:

export function usePromise<T>(
  pThunk: () => Promise<T>,
): [null | T, Error | null, boolean] {
  const [state, setState] = useState<T | null>(null);
  const [error, setError] = useState<Error | null>(null);
  const [loading, setLoading] = useState(true);
  useEffect(() => {
    let cancelled = false;
    pThunk()
      .then((result) => !cancelled && setState(result))
      .catch((error) => !cancelled && setError(error))
      .finally(() => !cancelled && setLoading(false));
    return () => {
      cancelled = true;
    };
  }, []);
  return [state, error, loading];
}

We’ve removed the promise from state, and instead we ask for a thunk, a function that returns a promise when called. Then, inside of the useEffect we call that thunk and register our promise callback handlers. The updated call site looks like this:

  const [subscription, subscriptionError] = usePromise(
    () => userStore.getActiveUserSubscription()
  );

It’s still very easy to call usePromise, but we’re not calling getActiveUserSubscription on each render anymore, just generating a function that can call it. And the function is only called once, inside of the useEffect in usePromise.

Sneaky bug, squashed 🐛


Posted

in

by

Tags:

Comments

Leave a Reply

Your email address will not be published. Required fields are marked *