configurable ping interval

[shinsuny]

No description provided.

[tjenkinson]

Like the ping interval option, but not sure about the other one

[tjenkinson]

I don't think this makes sense? The documentation says this does nothing except trigger the 'timeout' event which we are not handling

[shinsuny]

Yes, will remove this.

[tjenkinson]

I think this should be

typeof config !== 'string' && typeof config !== 'object'

[shinsuny]

see below

[tjenkinson]

It's better to throw an Error object so there's a stack trace.

throw new Error('...');

[shinsuny]

  if (config == null || ((typeof config) !== 'string' && config.hasOwnProperty('ip') === false)) {
    throw new Error('Invalid Configuration, please refer to documentations.');
  }

Thinking like this

[tjenkinson]

== null is OK because it will also catch 'undefined'

But I think it would be clearer though to be more explicit like

function isConfigValid(config) {
return (
typeof config === 'string' ||
(typeof config === 'object' && !!config.ip)
);
}

...

if (!isConfigValid(config))

[tjenkinson]

On phone and it's removed all the indentation

[shinsuny]

if config = null then typeof config === 'object' will be true and config.ip will throw exception

[tjenkinson]

Ah l see in which case

(typeof config === 'object' && && config !== null && !!config.ip)

[shinsuny]

if use config != null then npm test fails so did like below:

  /**
   * validate configuration
   * 
   * @param {*} config 
   */
  function isConfigValid(config) {
    return (
      config !== undefined &&
      config !== null &&
      (typeof config === 'string' || !!config.ip)
    );
  }

  // check for valid configuration
  if (!isConfigValid(config)) {
    throw new Error('Invalid Configuration, please refer to documentations.');
  }

[tjenkinson]

This can be a bit neater using Object.assign

var cfg = Object.assign({ pingInterval: 10000 }, config)

Where you would check if the provided config is a string and set it to { ip: config } above if it is

This also means we never mutate the input :)

[shinsuny]

var cfg = {};
if (typeof config === 'string') {
  cfg.ip = config;
}
cfg = Object.assign({ pingInterval: 10000, timeout: 3000 }, cfg);

Do you mean like this?

[tjenkinson]

Almost

if (typeof config === 'string') {
config = { ip: config };
}
config = Object.assign({ pingInterval: 10000, timeout: 3000 }, config);

[shinsuny]

you mean:

if (typeof config === 'string') {
config = { ip: config };
}
config = Object.assign({ pingInterval: 10000, timeout: 3000 }, config);

yes its better

[tjenkinson]

See other comment :)

[tjenkinson]

Looks great thanks! Will merge and publish a new version when I'm at a computer