Throttling promise execution (API requests, etc)

Question

I was working on a client for a remote api and realized that I needed to throttle requests at a maximum rate of 4 Hertz (4 requests per second). I wrote a simple typescript class to do it and wanted to solicit code review. It basically should accept a () => Promise<R> and return a Promise<R> that resolves once executed by the queue.

My prototype solution is here, and I'd just like to solicit feedback for it. Original code here.

/**
 * Request throttle has an add() method which takes a () => Promise<string>
 * and queues the promise to be executed in order.
 * add() returns a promise that resolves with the original promise result.
 */
class RequestThrottle {
  stack = [];
  spacing = 1000;

  add: (req: () => Promise<string>) => Promise<string> = req => {
    let executor;
    const requestPromise: Promise<string> = new Promise((resolve, _reject) => {
      let localExecutor = () => {
        resolve(req());
      };
      executor = localExecutor;
    });

    this.stack.push(executor);
    return requestPromise;
  };

  pullAndExecute = () => {
    const op: () => Promise<string> = this.stack.shift();
    // if (op) console.log("throttle found:", op);
    if (op) op();
  };

  interval = setInterval(this.pullAndExecute, this.spacing);

  stop = () => clearInterval(this.interval);
}

const throttle = new RequestThrottle();

/** 
 * Promise tester - to add a bit of extra async operations
 * (eg a network request to an api)
 * */ 
const addChild: (c: number | string) => Promise<string> = count => {
  const list = document.getElementById("list");
  const node = document.createElement("LI");
  node.innerHTML = count.toString();
  list.appendChild(node);
  const promise: Promise<string> = new Promise(resolve =>
    setTimeout(() => {
      log(`added "${count}"`);
    }, 500)
  );
  return promise;
};

const log = (s: string) => {
  const list = document.getElementById("log");
  const node = document.createElement("pre");
  node.innerHTML = s;
  list.appendChild(node);
};

addChild("Starting a List").then(console.log);

const enqueue: (i: number | string) => () => Promise<string> = i => {
  console.log("enqueueing " + i);
  return () => {
    return addChild(i);
  };
};

for (var i in [1, 2, 3, 4, 5, 6]) {
  throttle.add(enqueue(i)).then(console.log);
}

Answer 1

The most important advice I can offer is to turn on strictNullChecks and noImplicitAny. Without these settings, TypeScript's ability to warn you about problems in your code is severely limited.

Doing this first, TypeScript reveals several problems (Ignoring errors from your tests, you just need a ! assertion):

test.ts:19:21 - error TS2345: Argument of type 'undefined' is not assignable to parameter of type 'never'.

19     this.stack.push(executor);
                       ~~~~~~~~

test.ts:24:11 - error TS2322: Type 'undefined' is not assignable to type '() => Promise<string>'.

24     const op: () => Promise<string> = this.stack.shift();
             ~~

None of these errors are particularly difficult to fix. First, we need to define the type of stack. Right now, as it is assigned [], the inferred type is [] (an empty tuple). If we look at the type of functions added to it in add, we see that a function of type () => void is added, so declare that.

class RequestThrottle {
  stack: (() => void)[] = [];

This change will change the reported errors slightly, giving us more useful error messages.

test.ts:19:21 - error TS2345: Argument of type 'undefined' is not assignable to parameter of type '() => void'.

19     this.stack.push(executor);
                       ~~~~~~~~

test.ts:24:11 - error TS2322: Type '(() => void) | undefined' is not assignable to type '() => Promise<string>'.
  Type 'undefined' is not assignable to type '() => Promise<string>'.

24     const op: () => Promise<string> = this.stack.shift();
             ~~

The next problem is caused by let executor;. Since no type has been declared, TypeScript will implicitly type the variable as any. You could fix this by typing executor as (() => void) | undefined and then asserting that it has been set after executing the promise, but it is simpler to just add the function to stack within the promise.

At the same time, declaring the add function as a property instead of an arrow function on the class (like you did with pullAndExecute) is kind of weird. I'd prefer this style:

class RequestThrottle {
  stack: (() => void)[] = [];
  spacing = 1000;

  add = (req: () => Promise<string>): Promise<string> => {
    return new Promise<string>((resolve) => {
      this.stack.push(() => {
        resolve(req());
      });
    });
  };

This resolves the first issue, the second issue can be easily resolved by dropping the erroneous Promise<string> type on op in the pullAndExecute function.

class RequestThrottle {
  stack: (() => void)[] = [];
  spacing = 1000;

  add = (req: () => Promise<string>): Promise<string> => {
    return new Promise<string>((resolve) => {
      this.stack.push(() => {
        resolve(req());
      });
    });
  };

  pullAndExecute = () => {
    const op = this.stack.shift();
    if (op) op();
  };

  interval = setInterval(this.pullAndExecute, this.spacing);

  stop = () => clearInterval(this.interval);
}

Next we must consider the access level of each member. add should obviously be public. Similarly, it makes sense for stop to be public. But stack, spacing, interval and pullAndExecute are all implementation details. Users of this class shouldn't need to care (or even know) that they exist. They should all be marked as private.

Despite this... users might want to be able to specify spacing when using the class. There are two ways to meet this need. Either let the user specify it in the constructor, or add a start method that starts the interval. I prefer the start solution.

class RequestThrottle {
  private stack: (() => void)[] = [];
  private interval = 0;

  add = (req: () => Promise<string>): Promise<string> => {
    return new Promise<string>((resolve) => {
      this.stack.push(() => {
        resolve(req());
      });
    });
  };

  private pullAndExecute = () => {
    const op = this.stack.shift();
    if (op) op();
  };

  start = (spacing = 1000) => {
    this.stop();
    this.interval = setInterval(this.pullAndExecute, spacing);
  };

  stop = () => clearInterval(this.interval);
}

Note that in the start method I call stop first. This is to ensure that we don't have multiple queue consuming clocks running at once.

Now, you mentioned that you wanted this class to work with a Promise<R>. As written, it will only let you use Promise<string>. Thankfully, this is a trivial fix, we just need to make the add method generic.

  add = <R>(req: () => Promise<R>): Promise<R> => {
    return new Promise<R>((resolve) => {
      this.stack.push(() => {
        resolve(req());
      });
    });
  };

With all these changes together, the class becomes:

class RequestThrottle {
  private stack: (() => void)[] = [];
  private interval = 0;

  add = <R>(req: () => Promise<R>): Promise<R> => {
    return new Promise<R>((resolve) => {
      this.stack.push(() => {
        resolve(req());
      });
    });
  };

  start = (spacing = 1000) => {
    this.stop();
    this.interval = setInterval(this.pullAndExecute, spacing);
  };

  stop = () => clearInterval(this.interval);

  private pullAndExecute = () => {
    const op = this.stack.shift();
    if (op) op();
  };
}

A couple other notes:

1. Don't use for .. in to iterate over arrays. Use for .. of instead.

   for (const i of [1, 2, 3, 4, 5, 6]) {
     throttle.add(enqueue(i)).then(console.log);
   }
   

2. Don't make promises that can never be resolved or rejected. In addChild, the promise will never be resolved.

3. Should RequestThrottle throttle based on promise starts or promise resolves? Currently it throttles based on the initial call of a function... but if that function takes more than spacing to resolve, you could have multiple promises waiting at once. This might be desired, but it is something to consider.