-
-
Notifications
You must be signed in to change notification settings - Fork 444
#703 Fix unstable options object in useDebounceCallback breaking debouncing #704
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: master
Are you sure you want to change the base?
#703 Fix unstable options object in useDebounceCallback breaking debouncing #704
Conversation
|
…aks debouncing
a17cb4c
to
159f567
Compare
}, [func, delay, options]) | ||
}, [func, delay]) | ||
|
||
// Update the debounced function ref whenever func, wait, or options change | ||
useEffect(() => { | ||
debouncedFunc.current = debounce(func, delay, options) | ||
}, [func, delay, options]) | ||
}, [func, delay]) |
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.
How about instead of removing options changing it to memorization
const {leading, trailling, maxWait} = options
const memOptions = useMemo(() => ({leading, trailling, maxWait}), [leading, trailling, maxWait])
?
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.
This won't work.
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.
Ah, sorry, you suggest destructuring the options object and memoizing values individually.
I'll try.
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.
@Pasalietis I tried it and it does not work: other debounce tests start failing.

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.
Looks like a strange debounce
functionality when an empty options object is passed.
Old tests are passing with this:
export function useDebounceCallback<T extends (...args: any) => ReturnType<T>>(
func: T,
delay = 500,
- options?: DebounceOptions,
+ { leading, trailing, maxWait }: DebounceOptions = {},
): DebouncedState<T> {
const debouncedFunc = useRef<ReturnType<typeof debounce>>()
+ const memoizedOptions = useMemo(() => {
+ if (!leading && !trailing && !maxWait) return undefined
+
+ return ({ leading, trailing, maxWait })
+ }, [leading, trailing, maxWait])
useUnmount(() => {
if (debouncedFunc.current) {
@@ -85,7 +90,7 @@ export function useDebounceCallback<T extends (...args: any) => ReturnType<T>>(
})
const debounced = useMemo(() => {
- const debouncedFuncInstance = debounce(func, delay, options)
+ const debouncedFuncInstance = debounce(func, delay, memoizedOptions)
const wrappedFunc: DebouncedState<T> = (...args: Parameters<T>) => {
return debouncedFuncInstance(...args)
@@ -104,12 +109,13 @@ export function useDebounceCallback<T extends (...args: any) => ReturnType<T>>(
}
return wrappedFunc
- }, [func, delay, options])
+ }, [func, delay, memoizedOptions])
// Update the debounced function ref whenever func, wait, or options change
useEffect(() => {
- debouncedFunc.current = debounce(func, delay, options)
- }, [func, delay, options])
+ debouncedFunc.current = debounce(func, delay, memoizedOptions)
+ }, [func, delay, memoizedOptions])
return debounced
}
#703
This PR has two separate commits: