Problems
There was a bug in Booster.raf
in the else block you have this.run().bind(this)
and I assume you mean this.run.bind(this)()
though the binding is not needed because you are in the context of this
so this.run()
will do just fine.
You throw in two places yet ignore many problems that can be introduced by not vetting at the interface. There should be more checking when setting the Booster
state. Eg amount
should be a Number
, should not be less than 0. Booster.remove(n)
n could be a fraction, you should ignore or truncate floats.
It is possible for a function to execute even after Booster
is paused. This will happen if pause()
is called after the event is triggered. The run
function should make sure that you do not run in paused state.
If any of the given functions throw an error then all remaining function on the fn
array will not be executed. Nor do you check if any of the functions given are actually functions. This will also prevent functions after the bad function from being called.
You do not check if Booster.view
has the function addEventListener
. In Booster.throttle
this will result in the Booster
object incorrectly setting the active
state to true.
It is possible to have more than one event listener if Booster.init
is called more than once. Because you only have one Booster.timeout
property the Booster.throttle
function can overwrite the Booster.debounce
timeout property preventing debounce
from working correctly (and debounce
can stop throttle
timeouts breaking throttling)
Design
The logic is not fully thought out. It is always important to test your code for behaviour that are not expected.
I can not tell if you expect init
to be called again after construction. The fact that it is outside the constructor suggests so. If this is the case, why are you not providing a way to remove listeners. Calling init will just keep adding listeners and because the added listeners are anonymous there is no way to remove them.
I do not know how many events you are expecting to intercept or delay, but any more than a few and you would be much better of running a continuous rAF loop and handling all the needed events in that one function.
Review by comments.
All comments within /* ... */
are added as I went over the code, and other comments are me modifying your code by removal. The comments are generally unrelated suggestion and were created before I wrote the review above.
export default class Booster {
constructor(method, type, amount, ...fnArray) {
this.view = window;
this.type = type; /* type of what? bad naming maybe eventType */
this.amount = amount; /* amount? bad naming maybe bebounceDelay */
/* fn will always be an array as you have defined it as so in the arguments
this.fn = fn ? fn : []; // bad naming use plural or indicate its an array
Use existing methods */
// this.fnArray = fnArray;
this.add(fnArray);
/* Use existing methods */
//this.paused = false;
this.pause();
this.method = method; /* method? bad naming maybe methodType */
this.runBound = this.run.bind(this); /* Only need to bind once */
/* calling init in the constructor always seams such a silly thing to
do especially when init does not actually init the class */
/* Use a setter to set the method */
// this.init();
}
/* Use a setter to set method and remove init as it is not needed */
set method(type) {
if (type === 'throttle') { this.throttle(); }
else if (type === 'debounce') { this.debounce(); }
/* Why throw, You are not in control of the calling function so how do you know it
will be caught. Throwing will stop executing code and in production if
not caught end up in a non functioning page.
Degrade gracefully, don't fall in a heap at the slightest fault. */
else { console.warn("Invalid Booster method. Must be \"throttle\" or \"debounce\".") }
// else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
}
raf() {
if (this.view.requestAnimationFrame) {
this.view.requestAnimationFrame(this.runBound);
} else {
/* Run then bind ????? you had this.run().bind(this);
maybe you meant this.run.bind(this)(); */
this.run();
}
}
run() {
/* Don't need to check if zero. For each will do that for you */
// if (0 < this.fn.length) {
/* try to avoid callback iterators as they are slower than for loops */
//this.fn.forEach(function(fn) {
// fn();
//});
/* a better way to iterate
for(const fn of this.fnArray){ fn() } */
}
throttle() {
this.active = true;
this.view.addEventListener(this.type, () => {
if (this.active && !this.paused) {
this.active = false;
this.raf();
this.timeout = setTimeout( () => {
this.active = true;
}, this.amount);
}
}, false);
}
debounce() {
this.view.addEventListener(this.type, () => {
if (!this.paused) {
clearTimeout(this.timeout);
this.timeout = setTimeout( () => {
this.raf();
}, this.amount);
}
}, false);
}
add(...fnArray) { this.fnArray.push(...fnArray); }
remove(n) {
if (n >= 0 && n < this.fn.length) {
this.fn.splice(n, 1)
}
/* Again you are not in control of calling code and can not know
if this will be caught. I dont think this is even worth a warning */
// else {
// throw new Error('There are only ' + this.fn.length + ' functions in this Booster.');
//}
}
pause() { this.paused = true;}
resume() { this.paused = false; }
toggle() { this.paused = !this.paused; }
/* removed as not needed, see setter set method */
//init() {
// if ('throttle' === this.method) { this.throttle(); }
// else if ('debounce' === this.method) { this.debounce(); }
// else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
//}
}