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

Debounce #9252

Merged
merged 4 commits into from May 12, 2017
Merged

Debounce #9252

merged 4 commits into from May 12, 2017

Conversation

jridgewell
Copy link
Contributor

A performant debounce.

* @returns {function()}
*/
export function debounce(win, callback, minInterval) {
let locker = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why 0 instead of null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setTimeout returns a positive int, so I use 0 to mean no timeout scheduled. Just keeps the type the same in the JS engine.

}

return function(...args) {
timestamp = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

mmm, I believe it is a more desirable behavior if the very first call is a pass-thru and does not wait (or at least configurable)

let's also document the behavior of the first call regardless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the PositionObserver code, that'd be incorrect. We can add a flag once it's needed later.

It is documented. 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

documentation wasn't clear to me (since the last call didn't register to me to imply of course also the first call since there is no last call :) ) I think first call is special enough to deserve its own sentence.

PositionObserver definitely should act on the first onScroll event it receives immediately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree current PositionObserver usage doesn't need (do not want) the very first call to be handled differently. Let's make it configurable if requested later.

Copy link
Contributor

@aghassemi aghassemi May 11, 2017

Choose a reason for hiding this comment

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

ignore me, confused debounce with throttle (again) 😳
Is there a better common name/API for debounce out there? like timeoutAfterLastCall(listener, cb, timeout) or var fn = timeoutAfterLastCall(cb, timeout) ?

Copy link
Contributor

@aghassemi aghassemi May 11, 2017

Choose a reason for hiding this comment

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

I am worried that this API only works nicely when used directly as a listener but becomes fragile in other cases.
If we take the PositionObserver use-case, I assume it would becomes something like:

addEventListener('scroll', onScroll);

const debouncedScroll = debounce(onEnd, 500);
function onScroll() {
  isScrolling = true;
  debouncedScroll();
}

function onEnd() {
  isScrolling = false;
}

This looks a bit hard to read (what is even a clean name for variable that holds the return of debounce?) also a bit fragile (e.g. replacing debouncedScroll(); with debounce(onEnd, 500); will break the code).

Why not something like:

addEventListener('scroll', onScroll);

listenerTimeout(onScroll, 500).then(onEnd);

function onScroll() {
  isScrolling = true;
}

function onEnd() {
  isScrolling = false;
}

does this make sense or am I off base here? ( I do understand current API is common in JS libraries and works nicely for cases like addEventListener('scroll', debounce(onEnd, 500));)

Edit:
I guess for something like listenerTimeout(onScroll, 500).then(onEnd); to work, it needs to be able to spy on function calls to onScroll. This is going down a rabbit hole. Maybe correct answer is to fix the first "it is fragile" problem by using two listeners:

addEventListener('scroll', onScroll);
addEventListener('scroll', debounce(onEnd, 500));

anyway, would still love to hear your opinion though @jridgewell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soo mannyyy commentttts.

Is there a better common name/API for debounce out there?

debounce is the standard name, since Underscore released it some 6 years ago.

what is even a clean name for variable that holds the return of debounce?

callback => debouncedCallback, same as we use callback => boundCallback

also a bit fragile (e.g. replacing debouncedScroll(); with debounce(onEnd, 500); will break the code).

That's like replacing boundMethod() with method.bind(this). One creates a wrapped function, the other executes it.

Something like listenerTimeout(onScroll, 500).then(onEnd);

You're looking for an Observable, specifically a debounced one.

Maybe correct answer is to fix the first "it is fragile" problem by using two listeners:

That's basically what I was suggesting in the PositionObserver PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of debounce is very different:
https://css-tricks.com/the-difference-between-throttling-and-debouncing/

zhouyx
zhouyx previously requested changes May 11, 2017
Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

Please make sure all usage of rateLimit are renamed.

@zhouyx zhouyx dismissed their stale review May 11, 2017 01:37

nvm I found it

lannka
lannka previously requested changes May 11, 2017
@@ -24,7 +24,7 @@
* @param {number} minInterval the minimum time interval in millisecond
* @returns {function()}
*/
export function rateLimit(win, callback, minInterval) {
Copy link
Contributor

Choose a reason for hiding this comment

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

utils/function.js sounds like a too broad name, and eventually become a garbage bin for everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Propose something better?

Copy link
Contributor

Choose a reason for hiding this comment

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

rate-limit.js? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then why's debounce in there? And they're so similar they should be in the same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

rate-limit.js

  • function throttle()
  • function debounce()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jridgewell jridgewell merged commit 7aba540 into ampproject:master May 12, 2017
@jridgewell jridgewell deleted the debounce branch May 12, 2017 21:22
@jridgewell
Copy link
Contributor Author

Welp, it didn't let me put the link in the dismissal. #9252 (comment)

@lannka
Copy link
Contributor

lannka commented May 12, 2017

@jridgewell you missed my last comment:

My understanding of debounce is very different:
https://css-tricks.com/the-difference-between-throttling-and-debouncing/

@jridgewell
Copy link
Contributor Author

Where was that? Either way, the debounce described in that article and the one I've written are the same. It uses Underscore's _.debounce (which I helped maintain) which I rewrote here.

@lannka
Copy link
Contributor

lannka commented May 12, 2017

const remaining = minInterval - (Date.now() - timestamp);

My understand of debounce function will only fire when it has been idle (no calls) for the minInterval time. So why do we need to calculate remaining ?

@jridgewell
Copy link
Contributor Author

That's an optimization so we don't have to reset and queue a new timer every time (micro-slow). So:

  • timer is scheduled at t=20 and minInterval=100
  • second call comes in at t=50, but doesn't reset timer
  • we execute the timeout callback at t=120, but we have to wait 100 - (120 - 50) = 30ms
    • schedule a timer for 120 + 30 = 150
  • execute timeout at t=150

@lannka
Copy link
Contributor

lannka commented May 12, 2017

Got you. Sounds good.

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

Successfully merging this pull request may close these issues.

None yet

6 participants