-
Notifications
You must be signed in to change notification settings - Fork 511
fix(throttle, debounce): Fix throttle with edges: ['trailing'] behaving like debounce
#1532
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where throttle with { edges: ['trailing'] } was behaving like debounce, only firing once at the end instead of periodically during continuous calls. The fix implements throttle functionality by adding a maxWait option to debounce that enforces periodic execution, then simplifying throttle to use debounce with maxWait: throttleMs.
Key changes:
- Added
maxWaitoption todebounceto enforce maximum delay between invocations - Simplified
throttleimplementation to usedebouncewithmaxWait - Added test cases for
edges: ['trailing']andedges: ['leading']options
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/function/debounce.ts | Added maxWait option to enforce periodic execution, including new state tracking (pendingAt) and logic to invoke when max wait time is exceeded |
| src/function/throttle.ts | Simplified from custom implementation to thin wrapper around debounce with maxWait parameter |
| src/function/throttle.spec.ts | Added three new test cases validating throttle behavior with different edge configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (Date.now() - pendingAt >= maxWait) { | ||
| pendingAt = Date.now(); | ||
| func.apply(pendingThis, pendingArgs); | ||
| cancelTimer(); | ||
| schedule(); | ||
| return; | ||
| } |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maxWait logic doesn't reset pendingArgs after invoking the function. This could cause the function to be invoked again at the end of the debounce period with the same stale arguments.
After func.apply(pendingThis, pendingArgs) on line 167, you should reset pendingThis and pendingArgs to prevent duplicate invocations:
if (Date.now() - pendingAt >= maxWait) {
pendingAt = Date.now();
func.apply(pendingThis, pendingArgs);
pendingThis = undefined;
pendingArgs = null;
cancelTimer();
schedule();
return;
}…ving like debounce
cf53261 to
0c3f323
Compare
Summary
throttlewith{ edges: ['trailing'] }was behaving likedebounce(only firing once at the end)Problem
The original
throttleimplementation tried to enforce periodic execution by callingdebounced.cancel()afterthrottleMs, butcancel()only cancels without executing. This caused continuous calls to never fire.Solution
Added
maxWaitoption todebounceto enforce periodic execution, then simplifiedthrottleto usedebouncewithmaxWait: throttleMs.Implementation Considerations
There were two possible approaches:
maxWaittodebounce(chosen)throttleimplementation, no code duplicationdebouncepublic APIthrottleindependentlydebounceAPI changeI chose approach 1 because:
throttle = debounce + maxWaitis a common pattern (lodash does this)maxWaitis a useful option fordebounceusers as wellHowever, I'd like maintainer feedback on whether
maxWaitshould be:_maxWait)Changes
src/function/debounce.ts: AddedmaxWaitoption toDebounceOptionssrc/function/throttle.ts: Simplified to usedebouncewithmaxWaitsrc/function/throttle.spec.ts: Added test cases foredgesoptionsTest plan
edges: ['trailing']andedges: ['leading']options