fix: fix useDebounceCallback isPending logic #610
Conversation
I found a problem about the isPending response. It will return true after the first call. I think it should return false after there is no pending call👀
🦋 Changeset detectedLatest commit: a361251 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| debouncedFunc.current = debounce(parsedFunc, delay, options) | ||
| }, [parsedFunc, delay, options]) |
There was a problem hiding this comment.
I don't seem to understand how this hook's cancellation works.
We are debouncing debouncedFunc and debounced independently from each other, and return debounced.
So when component unmounts, debouncedFunc is cancelled and not debounced.
There was a problem hiding this comment.
Oh I got it, fixed it with the true ref 🤣
There was a problem hiding this comment.
I cannot see the purpose of debouncedFunc at all. It seems redundant since all operations could just be called on the debounced function created with useMemo.
The call to debouncedFunc.current.cancel() doesn't have the expected effect. When testing this hook, you can see that pending invocations will eventually execute even if the component has been unmounted. While this isn't the behavior I anticipated, it is reasonable. As a consumer, I would expect the debounced function to be cancelled (or flushed) only if I explicitly opt-in via a public, documented API.
Therefore, I believe this call (along all other usages of debouncedFunc) can be safely removed, as it has no effect and its removal simplifies the code without losing any necessary functionality.
Here is an example where I removed debouncedFunc altogether. However, I cancelled working on my PR because I figured that my change of cancelling or flushing on unmount would be an undesirable, breaking change. So please ignore the use of useUnmount and the added flushOnUnmount option.
There was a problem hiding this comment.
Thanks for your detailed review! That's a honor for me lol.
But I checked the cancel behavior is correct in this demo. https://stackblitz.com/edit/vitejs-vite-nwfbcy?file=src%2FTest.tsx
While you unmount the component, the debounced function just canceled. You can have a look in the demo console.
I think that's a reasonable behavior for the react users. Otherwise it maybe cause some memory leak or setState warning 👀
There was a problem hiding this comment.
That is a helpful demo for testing the cancel behavior. It shows that your variation of "useDebounceCallback" cancels on unmount. If you replace your hook with the original, you can see that the original does not have the cancel on unmount behavior. In my previous comment, I did not make it sufficiently clear that I was mostly referring to the "original" implementation and not your fix.
Here is my slightly edited variant of your demo, which uses the original implementation, showing that "called (true)" will be logged even if you unmount the component immediately after clicking.
This leads me to the conclusion that your current/previous implementation would be a breaking change in comparison to the current version. Therefore, it could only be released in a major version update if there is a good justification for the breaking change.
You could argue that "cancel on unmount" could be considered the "right behavior". However, "flush on unmount" could also be considered the "right behavior". Therefore, I believe that the way to go is to keep the API as minimal as it is (neither canceling nor flushing on unmount), thereby leaving it up to the consumer whether he wants to add "cancel on unmount" or "flush on unmount" behavior. In any client application, the "useDebounceCallback" hook could easily be wrapped in a custom variation of that hook that adds any of these behaviors. This way, the API stays minimal and extensible without enforcing specific behaviors the client might not want.
With the recent merge of my proposed changes into this PR, the implementation keeps the behavior of not canceling on unmount, while removing the (seemingly) redundant debouncedFunc. Here is a demo.
Regarding memory leaks, I don't have any concerns because lodash's debounce documentation does not have any indication that we would need to manually clean up any unused debounced functions. Therefore, I assume that the function can be garbage collected when we are not referencing it anywhere and it is not pending anymore.
There was a problem hiding this comment.
Oh I got your meaning. I agree with you. Just fixing the isPending state in this PR will be more reasonable. I'm ok with this! Thx for sharing your idea!
| const parsedFunc = useCallback( | ||
| (...args: Parameters<T>) => { | ||
| const result = func(...args) | ||
| isPendingRef.current = false |
There was a problem hiding this comment.
Would it make sense to put this line (which resets isPending to false) into a finally block, so that it even works as expected if func throws an exception?
There was a problem hiding this comment.
wow, that's coolll, fixed!
that test the `isPending` behavior
…is the current status-quo. Also changed the implementation to conform to this behavior. Also performed some minor refactoring to remove the use of useRef<ReturnType<typeof debounce>>() and useEffect() as a simplification
Add tests for useDebounceCallback
I found a problem about the isPending response. It will return true after the first call.
I think it should return false after there is no pending call👀
Here is the demo: use the fixed hook, and the response may be correct
https://stackblitz.com/edit/vitejs-vite-kbwkhh?file=src%2FApp.tsx
Before:
After:
