shiny icon indicating copy to clipboard operation
shiny copied to clipboard

Should replace JS Throttler and Debouncer with lodash versions

Open wch opened this issue 3 years ago • 1 comments

The TypeScript Throttler and Debouncer classes are difficult to reason about, and prone to bugs. See #3657 and #3659. After the 1.7.2 release, we should replace them with an implementation that uses the lodash throttle and debounce methods, which are well-tested.

The code for `Throttler will become much simpler:

class Throttler<X extends AnyVoidFunction> implements InputRatePolicy<X> {
  target: InputPolicy;
  func: X;
  delayMs: number | undefined;
  private throttledFn: DebouncedFunc<X>;

  constructor(target: InputPolicy, func: X, delayMs: number | undefined) {
    this.target = target;
    this.func = func;
    this.delayMs = delayMs;

    this.throttledFn = throttle(func, delayMs);
  }

  normalCall(...args: Parameters<X>): void {
    this.throttledFn(...args);
  }

  immediateCall(...args: Parameters<X>): void {
    this.throttledFn(...args);
    this.throttledFn.flush();
  }

  // Note: I don't think this method can be implemented
  // isPending(): boolean {
  // }

  flush(): { 
    this.throttledFn.flush();
  }
}

I don't think that isPending() can be implemented with lodash's throttle. However, we don't need it -- we simply need a flush() method. In the places where isPending() is used, it's followed by an .immediateCall(), as in:

    if (brushInfoSender.isPending()) brushInfoSender.immediateCall();

And that's equivalent to:

    brushInfoSender.flush()

It's also not clear to me why the Throttler class needs target, so maybe that can be removed as well. Or, perhaps it could be more general, with type Object instead of InputPolicy.

One more note: we might be able to remove the Throttler and Debounce classes entirely and just use lodash's functions directly (instead of wrapping them in these classes).

wch avatar Jul 06 '22 00:07 wch

FYI lodash has had numerous security vulnerabilities in the last few years (mostly prototype pollution iirc).

jcheng5 avatar Jul 06 '22 01:07 jcheng5