Skip to content

Fix middle functionality, add end option, migrate tests to fake timers#37

Open
iansan5653 wants to merge 13 commits into
github:mainfrom
iansan5653:main
Open

Fix middle functionality, add end option, migrate tests to fake timers#37
iansan5653 wants to merge 13 commits into
github:mainfrom
iansan5653:main

Conversation

@iansan5653

@iansan5653 iansan5653 commented Jul 22, 2026

Copy link
Copy Markdown
Member

#23 is occurring because the middle functionality is broken; it only tracks time since last attempt which means that it will never call the function during a sequence.

I've fixed this bug and refactored the logic in an attempt to make this more straightforward. While doing so, I also added an option for end to reach feature parity with Lodash. Note that Lodash calls this trailing but I used end to line up more with how we named start (instead of leading).

Finally I updated the tests to use useFakeTimers and not run in a Playwright browser; this allows them to complete very quickly. And I updated some incorrect test cases (see my PR comments).

Comment thread test/index.ts
Comment thread test/index.ts Outdated
Comment thread README.md Outdated
@iansan5653
iansan5653 marked this pull request as ready for review July 22, 2026 17:49
@iansan5653
iansan5653 requested a review from a team as a code owner July 22, 2026 17:49
@iansan5653
iansan5653 requested review from Copilot and dgreif July 22, 2026 17:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes throttle sequencing, adds trailing-call control, and speeds tests using fake timers.

Changes:

  • Refactors throttle timing and adds the end option.
  • Migrates tests from Playwright to Vitest fake timers.
  • Updates API documentation and marble diagrams.
Show a summary per file
File Description
index.ts Refactors timing and adds end.
test/index.ts Adds fake-timer and end tests.
README.md Documents updated behavior.
vitest.config.ts Disables browser testing.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

README.md:55

  • The same {start:false} timing applies when once is enabled: after the fix to once, the implementation invokes value 2 on the second attempt and then cancels. The diagram currently promises value 3, which is inconsistent with the documented call loop and implementation.
| throttle(fn, 100, {once: true, start: false})|     3                   |
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread index.ts Outdated
Comment thread README.md Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@dgreif

dgreif commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

TL;DR: I think we need to explicitly choose between Lodash-style trailing behavior and the burst-end behavior in this PR. The implementation currently chooses burst-end behavior. That is a reasonable model for independently configurable middle and end, but it is not equivalent to Lodash, and several claims and diagrams currently blur the difference.

With wait = 100 and attempts at 0ms and 50ms:

Implementation Callback invocations
This PR 1@0ms, 2@150ms
Lodash 1@0ms, 2@100ms

The model in this PR appears to be:

  • start: invoke the first attempt in a burst.
  • middle: invoke when another attempt arrives during the burst and enough time has passed since the previous callback.
  • end: keep the latest suppressed attempt pending, reset its timer on each subsequent attempt, and invoke it after a full wait period of inactivity. A successful middle call clears that pending end call.

That makes middle arrival-triggered and end inactivity-triggered. An end can fire once per burst, and then fire again for later bursts.

Lodash works differently. Its trailing call is tied to the current throttle window, not to inactivity after the latest attempt. It fires with the latest pending arguments at the throttle boundary. If activity continues afterward, that callback effectively becomes a middle callback in retrospect. Lodash therefore does not have a clean, knowable distinction between middle and end at invocation time.

This distinction also means the changed expectations from values like [1, 2, 4, ...] to [1, 3, 5, ...] encode a contract change, rather than only correcting broken tests. In a timer-based throttle, argument 2 may arrive at 50ms but be invoked at 100ms, so it was not necessarily invoked too quickly.

The reproduction in #23 also still produces only start and end calls with this implementation: attempts stop at 900ms, before the 1000ms middle threshold, and the end call runs around 1900ms. Lodash would run the pending trailing call around 1000ms.

Can we make the intended contract explicit before merging? If burst-end behavior is intentional, I think we should remove the Lodash parity claim, describe middle as the first qualifying incoming attempt, describe end as an inactivity-triggered flush of a suppressed attempt, and put actual timestamps in the diagrams. If Lodash behavior is intended, the trailing timer needs to follow the throttle window rather than restarting from the latest attempt.

Comment thread index.ts Outdated
@dgreif
dgreif requested review from a team and dgreif July 22, 2026 19:48
@iansan5653

iansan5653 commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

okay -- this took some thinking but I finally settled on an approach I'm sort-of-happy with.

The problem with just copying lodash is that lodash offers less flexibility: there's no middle option in lodash. So they can safely handle middle and end calls the same way, while we need to distinguish them.

However, Lodash spaces calls much more accurately than my initial approach. My approach treated wait as a minimum, but Lodash's middle-call-queueing logic ensures that calls are fired as close to every wait ms as possible during a sequence. I do like that and want to mimic it where possible.

So I took the lodash approach where I could, then took other approaches where I had to:

  • middle && end: Lodash. We can just queue every call when it comes in, since it's safe to treat middle and end calls the same. This is the default config and gets the best spacing.
  • middle && !end: We cannot queue the calls, because without predicting the future we cannot know when a call will be the last, which means we might accidentally make an 'end' call when we queued a 'middle'. So we have to wait to make 'middle' calls until the next call is made. This is like my first approach.
  • !middle && end: Again, without predicting the future we cannot know whether a call will be made after our queued up 'end' call (which would turn it into a 'middle' call which is disabled). So we have to wait the full wait time to ensure the sequence completely ends before we make an end call in this scenario. This is the debounce config.

Unfortunately this means special-casing each approach, but hopefully it's as close as possible to what consumers would expect.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for trailing:false The throttle is not performing as expected.

3 participants