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

refactor: replace _.debounce() with native equivalent #325

Closed
wants to merge 1 commit into from

Conversation

sockmaster27
Copy link
Contributor

No description provided.

return
}

// If we're not in cool down, call immediately and start cool-down
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right behavior we expect from debounce.

The point is that when you are typing we want to wait DELAY ms after the last keystroke.

We need to wait for the user to stop typing and then act.

We should NOT act on the first keystroke.

Copy link
Member

Choose a reason for hiding this comment

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

I think the correct behavior is to clear any existing timer and then schedule a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that completely kill auto completions?

Copy link
Member

@magnus-madsen magnus-madsen Feb 2, 2024

Choose a reason for hiding this comment

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

Isnt this the way it works now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, right now there is zero cooldown for typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, I understand that this implementation isn't equivalent to the _.debounce(). This is more of a rate limit.

I'm just questioning whether the old one would have worked if we were to enable it with the auto completions added.

Copy link
Member

Choose a reason for hiding this comment

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

So we have compileOnChangeDelay, but you are saying this value is always zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the change event we are passing in a skipDelay flag that circumvents the debouncing.

Copy link
Member

Choose a reason for hiding this comment

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

Roger.

Copy link
Member

Choose a reason for hiding this comment

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

I have talked with Matt about this. We will sleep on a bit, but we are thinking of just removing all this functionality.

No decision yet though.

@magnus-madsen magnus-madsen marked this pull request as draft February 2, 2024 08:55
@sockmaster27 sockmaster27 deleted the debounce branch February 3, 2024 09:17
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