Repository navigation
refactor: separate internal debounce and throttle hooks - #565
Conversation
Update callback, state, and value hooks to use internal implementations. Exclude internal hooks from public API JSDoc and export checks.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1-base #565 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 73 77 +4
Lines 2347 2505 +158
Branches 754 788 +34
==========================================
+ Hits 2347 2505 +158 🚀 New features to boost your workflow:
|
Antoliny0919
left a comment
There was a problem hiding this comment.
I left a few comments with thoughts and questions that came up while working on this.
Most are outside the scope of this PR, so please take a look and share your thoughts whenever you have a chance! 😁
| @@ -0,0 +1,93 @@ | |||
| // Simplified version of https://github.com/toss/es-toolkit/blob/main/src/function/debounce.ts | |||
There was a problem hiding this comment.
I noticed that we have tests for throttle, but none for debounce.
It looks like the debounce and throttle implementations were originally taken from es-toolkit and slightly adapted for simplikit.
Since we maintain this logic ourselves rather than delegating it to an external library, I think it would be good to add tests for debounce too :)
What do you think? I've left it as is for now, since it's outside the scope of this PR!
There was a problem hiding this comment.
Agreed, it would be great to have tests for debounce too 👍
Would you like to open a follow-up PR for it? If you'd rather not, no worries. We can track it in an issue 🙌
| import { DebouncedFunction } from './debounce.ts'; | ||
| import { useDebounce } from './useDebounce.ts'; | ||
|
|
||
| describe('useDebounce', () => { |
There was a problem hiding this comment.
Looking at useDebounce on its own, I think several test cases are missing.
The current tests mainly verify how many times the callback is called.
It would be good to also test whether pending calls are cancelled when the delay or options change, and whether subsequent calls use the latest values when different values are passed in.
I haven't added these tests, since they're outside the scope of this PR.
There was a problem hiding this comment.
Agreed. Testing that pending calls are cancelled when the delay or options change, and that later calls use the latest values, would be valuable 👍
It could go into the same follow-up PR as the debounce tests!
There was a problem hiding this comment.
Thanks! The missing tests aren’t urgent, so I’ll add them after the major version is released.
There was a problem hiding this comment.
I'm a little unsure where to put the internal hooks.
I initially considered the existing src/internal directory.
However, hooks would then depend on internal hooks, which in turn depend on usePreservedCallback from the hooks directory:
hooks/useDebouncedCallback → internal/useDebounce → hooks/usePreservedCallback
This doesn't create a circular dependency right now, but it seems like it could lead to one in the future. That's why I created a separate internal directory inside hooks.
However, this approach requires adding exclusions for that directory in parts of CI.
Would it be better to keep them in hooks/internal as they are now, or move them to the existing src/internal directory?
I'd appreciate your thoughts on this! 🙏
There was a problem hiding this comment.
For v1, we'd like to keep the implementation where it is (src/hooks/useDebounce, src/hooks/useThrottle) and keep using it internally. The public useDebounce / useThrottle will only be marked @deprecated in v1 (#537). In v2, we'll stop exporting them and move them under src/_internal/hooks, similar to es-toolkit's src/_internal.
Thank you for thinking this through with us and sharing your ideas! 👍
There was a problem hiding this comment.
Thank you for the clarification!
My understanding is that we'll keep useDebounce and useThrottle in their current locations (src/hooks/useDebounce and src/hooks/useThrottle), and hooks such as useThrottledCallback and useThrottledValue will continue referencing them as before.
For v1, the only change needed is to add @deprecated annotations to indicate that useDebounce and useThrottle will be removed from the public API in v2.
However, the current code references the implementations that were moved under _internal, and those changes have already been merged, so I'm not sure whether I've understood the plan correctly.
Is this something you plan to address as part of #537?
There was a problem hiding this comment.
Once I have a clearer understanding of the scope, I think I can work on #537 tomorrow 👍
There was a problem hiding this comment.
Currently, only the Callback hooks take all arguments in a single object, unlike the Value and State hooks. What do you think about accepting only leading and trailing in an options object, consistent with the other hooks?
This would be a breaking change, but I think it's worth addressing as we prepare for a major release. Users familiar with the Value or State hooks would likely expect the corresponding Callback hooks to have a similar interface.
Wouldn't making these interfaces consistent make the API more predictable for users?
There was a problem hiding this comment.
Sounds good! I'm not sure whether this should be included in #537, but I think it just needs to be done before v1 is released.
|
I didn't add a changeset since this doesn't change the public API or behavior. Please let me know if one is needed 👍 |
mnxmnz
left a comment
There was a problem hiding this comment.
Thanks for working on this! 👍 👍 👍
As you mentioned, your questions are outside the scope of this PR, so we'll discuss them among the maintainers first and follow up here with a summary. It would be great to continue the conversation from there.
Thanks again for raising these questions and thinking them through with us! 🙌
Overview
Closes: #559
Move the soon to be deprecated
useDebounceanduseThrottleimplementations into internal hooks.Their public APIs will still be deprecated as planned.(I think we only need to handle the deprecation in this issue. -> #537)
Use these internal hooks in the debounce and throttle related hooks(value, callback, state), preserving the existing behavior.
Checklist
yarn run fixto format and lint the code and docs?yarn run test:coverageto make sure there is no uncovered line?