useDebounce is broken
Closed this issue · 7 comments
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.
- Value updates. Timeout is scheduled.
X
is triggered. It rerenders the component synchronously.Y
is triggered, but for the closure of previous render.debouncedState[0]
andvalue
have different values.Y
reschedules a timeout. Remember that we are still in the call stack of "Step 1".- Funny enough,
Y
is triggered again. This time, it is for the closure of current render.debouncedState[0]
andvalue
have different values.Y
reschedules a timeout. This step is actually not needed for a bug to occur. Step 3 is enough. - Call stack rewinds up and we are back in "Step 1". The next line is
Z
. 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];
}
@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)
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 🎉
Good point! I'll move it.
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.
I'll close the issue. Feel free to create a new one or reopen this if you get any questions :)