feat(hooks): Add useDebounce Hook to Hooks #2561
Conversation
Deploying atlantis with
|
| Latest commit: |
7be8f5d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://525fd9b8.atlantis.pages.dev |
| Branch Preview URL: | https://export-use-debounce-hook-for.atlantis.pages.dev |
|
Published Pre-release for 69f7bac with versions: To install the new version(s) for Web run: To install the new version(s) for Mobile run: |
|
@nad182 definitely still relevant, possibly more relevant since I made it even! we have a the problem is that the callback you pass to it's too complicated to use, and none of that is communicated. the existing usages all don't work as intended due to this. looks like someone else needed a debounce hook for React and they copied that exact same hook, so now there are 2 versions of it, along with copies of a having a single React hook to handle debouncing will remove the need to have duplicated hooks and functions. then, this one actually works the way you'd expect. the one thing that is worth considering with this IMO is the |
There was a problem hiding this comment.
I've made 2 small suggestions.
But I have another one that is a suggestion/question: can we/should we replace the existing usages of debounce from "lodash" with this hook in the atlantis repo?
Also, regarding your comment whether using lodash is fine, I think it is! We're using several utilities provided by lodash in ~40 files. So it's not like we're adding a new dependency.
we should, but that's a lot more work than I want to do in this PR. some of those instances work, some of them don't. we need to go through them, and either fix them if they're broken which for at least one of them is non a trivial change, or move them over to use |
In general I'm in favour of any effort to reduce our reliance on lodash. |
|
the last commit changes how the hook works a bit. we're using
this was a suggestion from Eric, and he has implemented this in a temp version of testing it out, it continues to work as expected on the hook doc example, and Autocomplete where we use it for debouncing the search's onchange |
| wait: number, | ||
| options?: Parameters<typeof debounce>[2], | ||
| ) { | ||
| const funcRef = useCallbackRef(func); |
There was a problem hiding this comment.
Great improvement. Eric enlightened me roughly a year ago about not updating refs during rendering and I never got around to fixing this 👍
|
the only outstanding topic is replacing instances of lodash's debounce being used directly. I replaced the easy ones that were truly debouncing previously: Combobox, DataList and InputSearch. the last two, Chips and InputTime(s) are currently not working and somewhat rely on the broken behavior. a larger refactor is required for these that I'd rather not do right now. |
| expect(mockFn).toHaveBeenCalledWith("second"); | ||
| }); | ||
|
|
||
| it("should not recreate debounced function when options object reference changes", () => { |
There was a problem hiding this comment.
This is very close! I just gave it a test run to see if it would fail as expected, if options were included in the dependencies array. Unfortunately this still passes for me locally. Another test below actually does fail, which is great.
There's 2 easier ways we can verify the debounce function was not recreated.
- Compare func references haven't changed
const mockFn = jest.fn();
// Use a function that returns a new options object each time
const { result, rerender } = renderHook(
({ options }) => useDebounce(mockFn, DEBOUNCE_WAIT, options),
{ initialProps: { options: { maxWait: 1000 } } },
);
const debounceRef = result.current;
rerender({ options: { maxWait: 1000 } });
// The function's ref should never change
expect(debounceRef).toEqual(result.current);- Use the
leadingoption to determine if the config change took affect
const mockFn = jest.fn();
// Use a function that returns a new options object each time
const { result, rerender } = renderHook(
({ options }) => useDebounce(mockFn, DEBOUNCE_WAIT, options),
{ initialProps: { options: { leading: false } } },
);
result.current("first");
act(() => {
jest.advanceTimersByTime(1);
});
expect(mockFn).not.toHaveBeenCalled();
// This means it calls immediately at the leading edge of the timeout.
rerender({ options: { leading: true } });
result.current("second");
act(() => {
jest.advanceTimersByTime(1);
});
// The config change should be ignored, options are hardcoded
expect(mockFn).not.toHaveBeenCalled();Both of these pass locally when options is not a dep, and fail if it is 👍
There was a problem hiding this comment.
great suggestions. pushed these up!
There was a problem hiding this comment.
Everything else looks great, tested the updated components too 👌
Just the one test adjustment to ensure it fails if someone incorrectly adds options as a dependency 🙏
Motivations
we need a convenient hook for using debounce in React. we have one that is supposed to work elsewhere but it doesn't unless it's under very specific circumstances which is not particularly useful, whereas this hook is
for a quick version of why this hook is needed at all, it's because React re-makes things including functions on re-renders. the way debouncing works is that it wants to make sure the exact same function was called within the debounce window, and so those two things conflict making debouncing more complicated in React.
Changes
Added
useDebouncehook that was previously an internal util hook now available publicly along with some docs about it.Changed
Deprecated
Removed
Fixed
Security
Testing
verify debounce works on Combobox, DataList and components-native's InputSearch
if you simply put some logging in the method that is supposed to be getting debounced, if you type
asdfghjquickly you should only get that final result as the callback argument. not a whole bunch foraasasdasdfetc.you can also try out the hook somewhere new and exciting if you'd like.
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.