-
Notifications
You must be signed in to change notification settings - Fork 34
feat: Add cleanup support to debounce hook #379
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds cleanup support to the debounce utility to prevent memory leaks and post-unmount function execution. This addresses an issue where debounced functions were executing after component unmount, causing errors with disconnected DOM elements.
- Enhanced
debounce
function with acancel
method for clearing pending timeouts - Added automatic cleanup via
useEffect
inuseDebounce
hook to prevent post-unmount execution - Updated documentation to reflect the new cleanup functionality
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const debouncedFn = useMemo( | ||
() => | ||
debounce( | ||
(...args: Parameters<Callback>) => callbackRef.current(...args), | ||
delay, | ||
), | ||
[delay], | ||
) | ||
|
||
// Cancel pending timeouts on unmount | ||
useEffect(() => { | ||
return () => { | ||
debouncedFn.cancel() | ||
} | ||
}, [debouncedFn]) | ||
|
||
return debouncedFn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect dependency on debouncedFn
will cause the cleanup effect to re-run whenever the debounced function changes (when delay
changes). This creates and destroys cleanup handlers unnecessarily. Consider using useRef
to store the debounced function or move the cleanup logic to depend only on unmount.
const debouncedFn = useMemo( | |
() => | |
debounce( | |
(...args: Parameters<Callback>) => callbackRef.current(...args), | |
delay, | |
), | |
[delay], | |
) | |
// Cancel pending timeouts on unmount | |
useEffect(() => { | |
return () => { | |
debouncedFn.cancel() | |
} | |
}, [debouncedFn]) | |
return debouncedFn | |
const debouncedFnRef = useRef<ReturnType<typeof debounce>>() | |
useEffect(() => { | |
debouncedFnRef.current = debounce( | |
(...args: Parameters<Callback>) => callbackRef.current(...args), | |
delay, | |
) | |
return () => { | |
debouncedFnRef.current?.cancel() | |
} | |
}, [delay]) | |
return debouncedFnRef.current! |
Copilot uses AI. Check for mistakes.
Fixes EPICSHOP-63. The issue was that: Un-cancelled debounced function executes post-unmount, triggering a failed
insertBefore
on the disconnected Mux player web component.cancel
method to the debounced function to clear any pending timeouts.useEffect
hook to callcancel
when the component unmounts, preventing memory leaks.This fix was generated by Seer in Sentry, triggered automatically. 👁️ Run ID: 1693079
Not quite right? Click here to continue debugging with Seer.