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.

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

Conversation

robin-macpherson
Copy link
Contributor

@robin-macpherson robin-macpherson commented May 17, 2022

Description

  • A quick POC to look into simply creating our own custom hook for debouncing to partially remove our lodash dependency
  • Removes a couple of imports where we were brining in all of lodash instead of importing a specific function

Subtasks

  • I have added this PR to the Journaly Kanban project ✅
  • Write the hook
  • Test the hook in My Feed with <SearchInput>
  • Test the hook with visualViewportDiff in <Toolbar>
  • A bit of TypeScript massaging with the hook

Deployment Checklist

  • 🚨 Deploy code to stage
  • 🚨 Deploy code to prod

Migrations

No migs

Screenshots

N/A

@vercel
Copy link

vercel bot commented May 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
journaly ❌ Failed (Inspect) May 17, 2022 at 4:28PM (UTC)

@robin-macpherson
Copy link
Contributor Author

robin-macpherson commented May 17, 2022

Hey @Lanny ! Would you mind taking a quick look at this whenever you get a chance? Decided to put this together as part of the follow-up for #700 😊

clearTimeout(timeout.current)
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.

@@ -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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants