Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+44 −15
Open

Write Custom useDebouncedCallback Hook & Partially Remove Lodash Dependency #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8470a36
Create useDebouncedCallback.ts
robin-macpherson May 17, 2022
09f774e
Test out the useDebouncedCallback hook with Toolbar
robin-macpherson May 17, 2022
a89e573
Try out useDebouncedCallback hook in SearchInput
robin-macpherson May 17, 2022
42e99b2
Remove unnecessary lodash import
robin-macpherson May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions packages/web/components/Dashboard/Filters/SearchInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { useCallback, useState } from 'react'
import _ from 'lodash'
import React, { useState } from 'react'
import { useTranslation } from '@/config/i18n'
import useDebouncedCallback from '@/hooks/useDebouncedCallback'

type Props = {
defaultValue: string
Expand All @@ -18,12 +18,9 @@ const SearchInput: React.FC<Props> = ({ defaultValue, onChange, debounceTime })
setValue(defaultValue)
}

const debounceSearch = useCallback(
_.debounce((_searchVal: string) => {
onChange(_searchVal)
}, debounceTime),
[],
)
const debounceSearch = useDebouncedCallback((searchVal: string) => {
onChange(searchVal)
}, debounceTime)

const onSearchChange = (e: React.ChangeEvent<HTMLInputElement>): void => {
setValue(e.target.value)
Expand Down
2 changes: 0 additions & 2 deletions packages/web/components/Dashboard/MyFeed/MyFeed.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import Head from 'next/head'
import { useRouter } from 'next/router'
import { toast } from 'react-toastify'

import _ from 'lodash'

import { useTranslation } from '@/config/i18n'

import PostCard from '../PostCard'
Expand Down
13 changes: 8 additions & 5 deletions packages/web/components/JournalyEditor/Toolbar/Toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { options, isTableActive } from '../helpers'
import SwitchToggle from '@/components/SwitchToggle'
import { useTranslation } from '@/config/i18n'
import useIntersectionObserver from '@/hooks/userIntersectionObserver'
import useDebouncedCallback from '@/hooks/useDebouncedCallback'

type ToolbarProps = {
allowInlineImages: boolean
Expand Down Expand Up @@ -65,14 +66,16 @@ const Toolbar = ({
setViewportsDiff(visualViewport.offsetTop)
}

onVisualViewportChange()
const debouncedOnVisualViewportChange = useDebouncedCallback(onVisualViewportChange, 500)
Copy link
Contributor

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.


visualViewport.addEventListener('resize', onVisualViewportChange)
visualViewport.addEventListener('scroll', onVisualViewportChange)
debouncedOnVisualViewportChange()

visualViewport.addEventListener('resize', debouncedOnVisualViewportChange)
visualViewport.addEventListener('scroll', debouncedOnVisualViewportChange)

return () => {
visualViewport.removeEventListener('resize', onVisualViewportChange)
visualViewport.removeEventListener('scroll', onVisualViewportChange)
visualViewport.removeEventListener('resize', debouncedOnVisualViewportChange)
visualViewport.removeEventListener('scroll', debouncedOnVisualViewportChange)
}
}, [])

Expand Down
31 changes: 31 additions & 0 deletions packages/web/hooks/useDebouncedCallback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { useRef, useCallback } from 'react'

/**
* Returns a memoized function that will only call the passed function when it hasn't been called for the wait period
* @param callback The function to be called
* @param wait Wait period after function hasn't been called for
* @returns A memoized function that is debounced
*/
const useDebouncedCallback = <CallbackArgs extends any[]>(
callback: (...args: CallbackArgs) => void,
wait: number,
) => {
// Use a ref to store the timeout between renders
// and prevent changes to it from causing re-renders
const timeout = useRef()

return useCallback(
(...args) => {
const later = () => {
clearTimeout(timeout.current)
callback(...args)
}

clearTimeout(timeout.current)
Copy link
Contributor

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

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

timeout.current = setTimeout(later, wait)
},
[callback, wait],
Copy link
Contributor

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.

)
}

export default useDebouncedCallback