useDebounce is broken

[anilanar]

I'll try to explain what's going on. I copy pasted the source code and marked some lines with comments so that I can refer to them while explaining.

      functionTimeoutHandler.current = setTimeout(() => {
        if (!isComponentUnmounted.current) {
          // MARKER: X
          debouncedFunction.apply(null, arguments);
        }
        // MARKER: Z
        cancelDebouncedCallback();
      }, delay);

  React.useEffect(() => {
    if (debouncedState[0] !== value) {
      // MARKER: Y
      debouncedCallback[0](value);
    }
  });

Steps 2-6 happen in the same event loop.

1. Value updates. Timeout is scheduled.
2. X is triggered. It rerenders the component synchronously.
3. Y is triggered, but for the closure of previous render. debouncedState[0] and value have different values. Y reschedules a timeout. Remember that we are still in the call stack of "Step 1".
4. Funny enough, Y is triggered again. This time, it is for the closure of current render. debouncedState[0] and value have different values. Y reschedules a timeout. This step is actually not needed for a bug to occur. Step 3 is enough.
5. Call stack rewinds up and we are back in "Step 1". The next line is Z.
6. Z is triggered, cancelling all previously scheduled timeouts in steps 3 and 4.

Possible solution:

First, I suggest using eslint-plugin-react-hooks and sticking to its rules. Otherwise it's too easy to make mistakes. Second, I think trying to save a few hundred bytes is unnecessary. It takes a lot away from code readability.

export default function useDebounce(value, delay, options = {}) {
  const [state, dispatch] = React.useState(value);
  const [callback, cancel] = useDebouncedCallback(useCallback((value) => dispatch(value), []), delay, options);

  React.useEffect(() => {
    callback(value);
  }, [callback, value]);

  return [state, cancel];
}

[xnimorz]

@anilanar Thank you for the issue!
I'll add the eslint-plugin-react-hooks and fix this issue a bit later (within 1-2 days)

[anilanar]

I think cancelDebouncedCallback() must be moved before debouncedFunction.apply(null, arguments); in both timeout handlers too. Because there's no guarantee that debouncedFunction.apply(null, arguments) won't schedule a rerender + another timeout synchronously.

Thanks for maintaining this library 🎉

[xnimorz]

Good point! I'll move it.

[xnimorz]

I've made the example https://codesandbox.io/s/cache-itq1l to check if we have excess function calls and I have published 2.0.1 version with fix and eslint config.

[xnimorz]

Hi, @anilanar .
Can we close the issue?

[anilanar]

@xnimorz I'll test this as soon as possible and let you know.

[xnimorz]

I'll close the issue. Feel free to create a new one or reopen this if you get any questions :)