Sending periodic heartbeats, wondering if there are any timer bugs

What?

heartbeatjs is a small light weight library that helps you run periodic heartbeat functions and detects timeouts when they occur by launching events.

It was mainly designed for tcp/ip connections, but you can use it with any protocol you want as it designed to be generic.

Project

The project page, with examples and documentation can be seen here:

https://fl4m3ph03n1x.github.io/heartbeatjs/index.html

Code

This library is mostly a collection of getters and setters mixed with some timers. I would like a second opinion regarding the code and any possible bugs one may find, as I am about to release the next version next week.

const isFunction = require("lodash.isfunction");


const DEFAULT_TIMEOUT = 5000;
const DEFAULT_INTERVAL = 3000;

const heartBeatFactory = () => {

    let interval = DEFAULT_INTERVAL,
        timeout = DEFAULT_TIMEOUT,
        ping,
        pong,
        timer,
        lastHeartbeatTime,
        timeoutTimer,
        hasStarted = false;

    const events = {
        timeout: () => {}
    };

    const hasTimedOut = () =>
        Date.now() - lastHeartbeatTime > timeout;

    const getBeatInterval = () => interval;

    const setBeatInterval = newInterval => {
        if(isNaN(newInterval))
            throw new TypeError(`${newInterval} must be a Number.`);

        interval = newInterval;
    };

    const getBeatTimeout = () => timeout;

    const setBeatTimeout = newTimeout => {
        if(isNaN(newTimeout))
            throw new TypeError(`${newTimeout} must be a Number.`);

        timeout = newTimeout;
        clearTimeout(timeoutTimer);
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const getPing = () => ping;

    const setPing = newPing => {
        ping = newPing;
    };

    const getPong = () => pong;

    const setPong = newPong => {
        pong = newPong;
    };

    const receivedPong = () => {
        lastHeartbeatTime = Date.now();
        clearTimeout(timeoutTimer);
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const stop = () => {
        lastHeartbeatTime = undefined;
        clearInterval(timer);
        timer = undefined;
        clearTimeout(timeoutTimer);
        timeoutTimer = undefined;
    };

    const start = fn => {
        if (!isFunction(fn))
            throw new TypeError(`${fn} must be a function.`);

        hasStarted = true;
        lastHeartbeatTime = Date.now();
        timer = setInterval(fn, getBeatInterval());
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const onTimeout = fn => {
        if (!isFunction(fn))
            throw new TypeError(`${fn} must be a function.`);

        events.timeout = fn;
    };

    const isBeating = () => timer !== undefined;

    const reset = () => {
        if (isBeating())
            stop();

        if( hasStarted ){
            setBeatInterval(DEFAULT_INTERVAL);
            setBeatTimeout(DEFAULT_TIMEOUT);
            ping = undefined;
            pong = undefined;
            onTimeout(() => {});
        }
    };

    return Object.freeze({
        getBeatInterval,
        setBeatInterval,
        getBeatTimeout,
        setBeatTimeout,
        hasTimedOut,
        getPing,
        setPing,
        getPong,
        receivedPong,
        setPong,
        stop,
        start,
        reset,
        isBeating,
        onTimeout
    });
};

module.exports = heartBeatFactory;

Just wondering why you don’t set hasStarted = false either in the stop() or in the reset() method.

By doing it in the stop() you could omit the second if in the reset() method.

Seeing a reset() method at least screams for resetting all values to default.

TL;DR: The code is overkill.

const isFunction = require("lodash.isfunction");

Why do you even need this. It’s an unnecessary dependency when you can simply use typeof and check if the result is function.

let interval = DEFAULT_INTERVAL,
    timeout = DEFAULT_TIMEOUT,
    ping,
    pong,
    timer,
    lastHeartbeatTime,
    timeoutTimer,
    hasStarted = false;

I usually recommend a var/let/const per variable. Saves you from those pesky trailing commas. And if you’re wondering “Oh, but with one each, I still need to use ;“, semi-colons are optional as well. The instances of you having to use ; that actually does something is very narrow.

if(isNaN(newInterval))
    throw new TypeError(`${newInterval} must be a Number.`);

Runtime type safety is just unnecessary overhead. I suggest you just allow the code to blow up if the consumer uses the wrong type. That’s enough for them to know they need to fix something. If you want real type safety, use a typed language like TypeScript, or if you can’t have TS, use a combination of JSDoc comments and a linter that uses the type definitions to hint types.

return Object.freeze({
    getBeatInterval,
    setBeatInterval,
    getBeatTimeout,
    setBeatTimeout,
    hasTimedOut,
    getPing,
    setPing,
    getPong,
    receivedPong,
    setPong,
    stop,
    start,
    reset,
    isBeating,
    onTimeout
});

Freezing an object is also unnecessary overhead. There’s little gain in freezing the instance properties. The consumer of the library probably has zero intention of breaking your objects if they decided to use it.

const heartBeatFactory = () => { }

Factories are great when they return data, because every return value is a unique value every time. But using factories to create objects with shared behavior like methods is not efficient. Everytime you call the factory, you’re creating duplicates of the same functions every time. I suggest you use constructors/classes instead. That way, data is per-instance while the methods are on the prototype, shared by all instances.

An added bonus to classes is that you get getter/setter syntax for free.

class HeartBeat {
    constructor (){
        this.interval = 3000
        this.timeout = 5000
        this.ping = null
        this.pong = null
        this.timer = null
        this.lastHeartBeatTime = null,
        this.timeoutTimer = null
        this.hasStarted = false
    }

    get hasTimedOut(){
        return Date.now() - this.lastHeartBeatTime > timeout
    }

    get beatInterval(){
        return this.interval
    }

    ... and so on.
}

const heartBeatInstance = new HeartBeat()
console.log(heartBeatInstance.hasTimedOut)

And while it’s a great addition to the language, I often find accessors overkill, especially if all they do is only get/set data. It’s probably better to have a public property. And the nice thing about JS getters/setters is that to the public, they’re read from and assigned to like regular object properties. Should you transition from public properties to getters/setters, all you need is expose a getter/setter of the same name, and rename the underlying variable.