-
Notifications
You must be signed in to change notification settings - Fork 18
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
Write Custom useDebouncedCallback
Hook & Partially Remove Lodash Dependency
#788
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
clearTimeout(timeout.current) | ||
timeout.current = setTimeout(later, wait) | ||
}, | ||
[callback, wait], |
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.
because callback
here is in the dependency array, to achieve a stable value the caller will have to memoize their function (likely using useCallback
) before passing it to this function, so we don't achieve much by the useCallback
call.
Since callers will have to memoize anyway (and the useRef
here can be accomplished with just an object declared outside the returned function) I'd suggest making this simply be a function rather than a hook that callers can use in a useMemo
.
@@ -65,14 +66,16 @@ const Toolbar = ({ | |||
setViewportsDiff(visualViewport.offsetTop) | |||
} | |||
|
|||
onVisualViewportChange() | |||
const debouncedOnVisualViewportChange = useDebouncedCallback(onVisualViewportChange, 500) |
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.
Calling a hook inside a useEffect
callback is invalid, which contributes to the case for this being a simple function rather than a hook.
callback(...args) | ||
} | ||
|
||
clearTimeout(timeout.current) |
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.
So the behavior that's implemented here is that any call to the debounced function will wait at least wait
ms before invoking the wrapped function, and no invocation will happen if another call to the debounced function happens before invocation of the wrapped func.
This is apparently lodash's definition of "debounce", but I think the behavior we actually want is what lodash calls "throttle", meaning the function will be invoked at most once in a period of wait
ms, ensuring the last call to the throttled function invoke the wrapped function as soon as wait
ms has elapsed since the last invocation.
So the logic for that would be something like:
- check the time since the last invocation of the wrapped function
- If it's more than
wait
ms ago, invoke again and update the last-invoked time - If it's less than
wait
ms ago:- Cancel any pending calls (
clearTimeout()
) - Create a new pending call scheduled for
wait - (now - lastInvocation)
ms in the future- When that pending call does eventually execute, update the last-invoked time
- Cancel any pending calls (
This is based on the assumption that in the scrolling case, we do want to continue to call this function while scroll events are happening and continue updating the toolbar position, not wait until all the scrolling action has stopped, we just want to limit the frequency at which that happens
Description
lodash
dependencylodash
instead of importing a specific functionSubtasks
<SearchInput>
visualViewportDiff
in<Toolbar>
Deployment Checklist
Migrations
No migs
Screenshots
N/A