Skip to content
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

Add maxWait option #12

Merged
merged 8 commits into from
Feb 28, 2021
Merged

Add maxWait option #12

merged 8 commits into from
Feb 28, 2021

Conversation

Julusian
Copy link
Contributor

@Julusian Julusian commented Jan 18, 2021

This adds a maxWait option.

The intention is to ensure that the callback is fired after at most maxWait of time.

This is particularly useful in the following situation. Perhaps this is streaming data of an volume control changing, but the source is sending updates too often.
wait=100, maxWait=1000
With the debounce being called every ~50ms for 30s.
Without this, only one update will be processed at the very end, rather than one every second.

index.d.ts Outdated
@@ -7,6 +7,15 @@ declare namespace debounceFn {
*/
readonly wait?: number;

/**
Maximum time to wait until the `input` function is called.
Copy link
Owner

Choose a reason for hiding this comment

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

It needs a clearer description of how it's different from wait.

Copy link
Owner

Choose a reason for hiding this comment

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

It would also be useful to include a use-case for this option (like you mention in the PR description).

Copy link
Owner

Choose a reason for hiding this comment

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

You also need to update the readme.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, make it clear that "time" is in milliseconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to improve this. I'm not sure the first line is any better, but I don't how to make it better

index.d.ts Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title feat: Add maxWait option Add maxWait option Jan 22, 2021
Base automatically changed from master to main January 23, 2021 09:08
@sindresorhus
Copy link
Owner

It's still not clear to the user how maxWait differs from wait. Maybe take a note of how Lodash documents their maxWait option.

index.d.ts Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
@Julusian
Copy link
Contributor Author

Ive tried again to reword it based on what lodash say

readme.md Outdated
##### maxWait

Type: `number`\
Default: `0` (disabled)
Copy link
Owner

Choose a reason for hiding this comment

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

Readme and index.d.ts should be in sync.

@sindresorhus sindresorhus merged commit 8eb3bcf into sindresorhus:main Feb 28, 2021
@sindresorhus
Copy link
Owner

Thanks :)

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.

2 participants