0
\$\begingroup\$

I have the following functions for polling some function periodically with a timeout

export function promiseTimeout(ms: number): Promise<NodeJS.Timer> {
  return new Promise<NodeJS.Timer>((resolve) => {
    return setTimeout(resolve, ms);
  });
}

export function poll(predicate: () => boolean, delayMS: number, timeoutMS: number): Promise<void | NodeJS.Timer> {
  const fixedDelayPoller: Promise<void> = new Promise<void>(async (): Promise<void> => {
    while (!predicate()) {
      await promiseTimeout(delayMS);
    }
  });
  return Promise.race([promiseTimeout(timeoutMS), fixedDelayPoller]);
}

I'm new to Typescript and Promises so I'm curious if there is a more idiomatic or flexible way for this to be written. For example, should there be parameters for a callback to be called after the timeout? Or would that be handled by the caller calling poll(x, y, z).then(onFulfilled)? Further, should this function return a boolean rather than a possible void, false in the case of timeout and true otherwise?

\$\endgroup\$
  • \$\begingroup\$ The Code review site is for code that already works (so you should have tested it already and should not be asking if it works) that you're interested in improving. \$\endgroup\$ – jfriend00 May 29 '18 at 23:00
  • \$\begingroup\$ @jfriend00 Sure. \$\endgroup\$ – geofflittle May 29 '18 at 23:07
  • \$\begingroup\$ It seems like your timeout needs to either reject with a known error that calling code can test for or resolve with a know value in a way that the timeout can be detected. When using a timeout of any kind, the caller typically needs to know if the operation succeeded or timed out. The way you're coding it, I don't see how the caller can easily get that information. Usually timeouts reject and can be detected in a .catch(). \$\endgroup\$ – jfriend00 May 29 '18 at 23:21
2
\$\begingroup\$

Polling is not good.

I'm curious if there is a more idiomatic or flexible way for this to be written.

Generally polling for a state change is not considered best practice so there is no idiomatic approch.

The code

  • async functions are a promise you don't need to put them inside a promise, you are just doubling up.
 const fixedDelayPoller = new Promise(async () => {
     while (!predicate()) {
         await promiseTimeout(delayMS); 
     }
 });

Then the same as with one less promise to negotiate.

 const fixedDelayPoller = (async () => {
     while (!predicate()) {
         await promiseTimeout(delayMS); 
      }
 })(); // invoke async function
  • It seams that JS coders are still getting used to promises as they never name the callbacks. You name the callback resolve but that is inferred and does not describe what the function does
  return new Promise((resolve) => {
    return setTimeout(resolve, ms);
  });

would be better as

  return new Promise(timeup => setTimeout(timeup, ms))
  • You are creating way too many promises. When you look at the problem you only need one promise that resolves on timeout or predicate() is true. As your code stands you create 5 promises (Promise.race counts as a promise) minimum and then an extra one for each delay timeout.

    A better approch is to have the poll function return only one promise. You can then resolve it without the need to create a string of promises one after another.

function poll(predicate, delayMS, timeoutMS){
    var intervalHandle;
    return new Promise(pollingOver => {
        const complete = () => {
            clearInterval(intervalHandle);
            pollingOver();
        }
        intervalHandle = setInterval(() => predicate() && complete(), delayMS);
        setTimeout(complete, timeoutMS);
    });
}

function poll(predicate, delayMS, timeoutMS){
    var intervalHandle;
    return new Promise(pollingOver => {
        const complete = (ready = false) => {
            clearInterval(intervalHandle);
            pollingOver(ready);
        }
        intervalHandle = setInterval(() => predicate() && complete(true), delayMS);
        setTimeout(complete, timeoutMS);
    });
}


// Poll waits fot state to === true or two seconds
test();
var state;
function test() {
    state = false;
    var timer = setTimeout(()=>state = true, Math.random() * 4000)
    poll(() => state, 200, 2000).then(inTime => {
        console.log(inTime ? "State in time" : "State change timed out");
        clearTimeout(timer);
        test();
    });
}

Back to your questions

I'm curious if there is a more idiomatic or flexible way for this to be written. For example, should there be parameters for a callback to be called after the timeout? Or would that be handled by the caller calling poll(x, y, z).then(onFulfilled)? Further, should this function return a boolean rather than a possible void, false in the case of timeout and true otherwise?

If you did need some event to happen on resolution then using poll(...).then( is the better approch.

Should your function return a bool? that is up to you. The predicate has access to the state so it can be used to workout the current state. However it is better to return a result via the promise callback (see snippet above, returns false on timeout)

Why are you polling?

I am having trouble understanding why you need to poll some state. There are many other ways to communicate state changes in JavaScript (TypeScript) without the need for code to run at regular intervals. But without some context it is hard to say which approch would be best.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.