Object.is equality check in react hook is causing multiple versions of the same function

Viewed 310

I'm not sure what the correct fix is to stop the following scenario. I've created this codesandbox to highlight the problem.

I have this hook and here is a scaled down version:

export const useAbortable = <T, R, N>(
  fn: () => Generator<Promise<T>, R, N>,
  options: Partial<UseAbortableOptions<N>> = {}
) => {
  const resolvedOptions = {
    ...DefaultAbortableOptions,
    ...options
  } as UseAbortableOptions<N>;
  const { initialData, onAbort } = resolvedOptions;
  const initialState = initialStateCreator<N>(initialData);
  const abortController = useRef<AbortController>(new AbortController());
  const counter = useRef(0);

  const [state, dispatch] = useReducer(reducer, initialState);

  const runnable = useMemo(
    () =>
      makeRunnable({
        fn,
        options: { ...resolvedOptions, controller: abortController.current }
      }),
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [counter.current]
  );

  const runner = useCallback(
    (...args: UnknownArgs) => {
      console.log(counter.current);

      dispatch(loading);

      runnable(...args)
        .then(result => {
          dispatch(success<N>(result));
        })
        .finally(() => {
          console.log("heree");
          counter.current++;
        });
    },
    [runnable]
  );

  return runner;
};

The hook takes a function and options object and as they are recreated on each render, and the hooks use Object.is comparison, it was creating a new version of the returned function no matter what I do.

So I have hacked it like this, to use a counter:

  const runnable = useMemo(
    () =>
      makeRunnable({
        fn,
        options: { ...resolvedOptions, controller: abortController.current }
      }),
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [counter.current]
  );

I have had to silence the linter to make this possible.

What the linter suggests is this:

  const runnable = useMemo(
    () => makeRunnable({ fn, options: { ...resolvedOptions, controller: abortController.current } }),
    [fn, resolvedOptions],
  );

But fn and resolvedOptions are causing a new runnable function to be created each time.

It is a real pain having to wrap everything in useCallback, useMemo, and friends.

I've had a look at other fetch libraries and they are doing other things like this, like JSON.stringify dependency arrays to get around this problem.

I like hooks but the Object.is equality check is killing the whole paradigm.

What would be the correct way for me to use the dependency array correctly so I don't get a new function each time and also keep the linter happy? The 2 requirements seem to add odds with each other.

3 Answers

You must note that you need not provide any hack to get around the callback problem

ESLint warnings about missing dependency are there to help users avoid mistakes make unknowingly and not to be enforced.

Now in your case

If you look at your useAbortable function, you are passing a generator function and an options object, now both of them are created on every re-render.

You can memoize the options and function passed to useAbortable to avoid issues with dependencies

  • If you use callback pattern for setMessages, you can create onAbort once only by providing [] dependency to useCallback

  • The generator function depends on a time delay from state so you can create it with useCallback and provide delay as a dependency

      const onAbort = useCallback(() => {
        setMessages(prevMessage => (["We have aborted", ...prevMessage]));
      }, []); // 


      const generator = useCallback(
        function*() {
          const outsideLoop = yield makeFetchRequest(delay, "outside");

          processResult(outsideLoop);

          try {
            for (const request of requests) {
              const result = yield makeFetchRequest(delay, `${request.toString()}`);

              processResult(result);
            }
          } catch (err) {
            if (err instanceof AbortError) {
              setMessages(["Aborted"]);
              return;
            }
            setMessages(["oh no we received an error", err.message]);
          }
        },
        [delay, processResult]
      );

      const options = useMemo(() => ({ onAbort }), [onAbort]);
      const { run, state, abortController, reset, counter, ...rest } = useAbortable<
        Expected,
        void,
        Expected
      >(generator, options);

Now inside useAbortable you need not worry about fn or options changing as they will only change when they absolutely have too if we implement it like above

So you your runnable instance in useAbortable can be clearly created with the correct dependencies

const resolvedOptions = useMemo(() => ({
    ...DefaultAbortableOptions,
    ...options
  }), [options]);

  const runnable = useMemo(
    () =>
      makeRunnable({
        fn,
        options: { ...resolvedOptions, controller: abortController.current }
      }),
    [fn, resolvedOptions]
  );

Working demo

The problem is with resolvedOptions and fn and not with runnable. You should rewrite runnable dependencies as the linter suggests and modify resolvedOptions to also be a memoized.

// I assume that `DefaultAbortableOptions` is defined outside of the `useAbortable` hook
const resolvedOptions = useMemo(() => ({
  ...DefaultAbortableOptions,
  ...options
}), [options]};

The hook takes a function and options object and as they are recreated on each render, and the hooks use Object.is comparison, it was creating a new version of the returned function no matter what I do.

When you consume your hook you should also pay attention to not recreate stuff unnecessarily and use useMemo and useCallback for data and functions defined inside your components.

function MyComponent () {
  const runnable = useCallback(() => {}, [/*let the linter auto-fill this*/])
  const options = useMemo(() => ({}), [/*let the linter auto-fill this*/])
  const runner = useAbortable(runnable, options)
}

This way the runner function will only be recreated when it really needs to be (when the dynamic dependencies of runnable and options change).

You have to go all-in with hooks if you use them and really wrap everything in them to make things work without shady bugs. I personally dislike them because of these traits.

Extra note: as the React docs point out, useMemo is not guaranteed to run only when its dependencies change, it may run on any render and cause runner to be recreated. In the current version of React this does not happen but it may happen in the future.

Actually, there is no need to wrap everything in useCallback, useMemo and etc.

About useCallback when it is necessary to use if we have an optimized child component and we pass an inside-function as a prop to it, then in each re-render, all variables and functions in the execution context of the function component are evaluated and re-created, so the child component mistakenly thinks the passed prop has changed but it doesn't have any changes it just re-created. so we use useCallback to memoize the inside-function and pass an array of dependency to really re-created it but it is obvious in each re-render JavaScript re-evaluated the function and it is inevitable.

And about the useMemo, you correctly use it for your case and this hook is for the memoizing variables in each re-render that it is needed to change on some dependencies, maybe these dependencies are not the things that the linter wants. you truly use your dependency, the counter.current for your desire.

The linter rule, hence react-hooks/exhaustive-deps is set for the below sample:

function Counter() {
  const [count, setCount] = useState(0);

  useEffect(() => {
    const id = setInterval(() => {
      setCount(count + 1); // This effect depends on the `count` state
    }, 1000);
    return () => clearInterval(id);
  }, []); // linter error: Obviously `count` is a dependency to this current useEffect

  return <h1>{count}</h1>;
}

So for having better use and omitting the dependency and have happy linter it is better to write the above code like the following:

function Counter() {
  const [count, setCount] = useState(0);

  useEffect(() => {
    const id = setInterval(() => {
      setCount(c => c + 1); // This line doesn't depend on `count` variable outside
    }, 1000);
    return () => clearInterval(id);
  }, []); // happy linter: because there is no dependency

  return <h1>{count}</h1>;
}

As you see the setCount uses an internal option to read the current state and by passing a function instead of an expression omit the dependency. so it can be inspiring in your case, I suggest two solutions:

  1. There is no difference between directly passing a function to useMemo or creating it by a function expression definition, so do it:

    const runnableFunction = () =>
      makeRunnable({
        fn,
        options: { ...resolvedOptions, controller: abortController.current }
      });
    
    const runnable = useMemo(
      runnableFunction,
      [counter.current]
    );
    

    By using this hack JavaScript doesn't sense change and the linter is happy, also, your correct usage of hooks still remains too.

  2. The fn comes from custom hook argument and the resolvedOptions made by options that comes from custom hook argument too and both of them maybe will have changes in the future. so by the emphasis of ReactJS docs and the emphasis of Dan Abramov, to use this rule pass the linter required dependencies plus your desired dependencies together:

    const runnableFunction = () =>
      makeRunnable({
        fn,
        options: { ...resolvedOptions, controller: abortController.current }
      });
    
    const runnable = useMemo(
      runnableFunction,
      [counter.current, fn, resolvedOptions]
    );
    

    I guess it is better to pass your desired dependencies at first.

For your current case, I recommended using the first one, but if the question is generic I recommended the second solution. it is recommended by docs.

Note: if you use the second solution, be aware this is an optimized custom hook function, so when you using it anywhere, you should pass a memorized fn and options to it. just like docs guidance.

Related