Suggested improvements

[lucasvdh]

Description

First of all, thanks for this great package. I've been using it recently for a home automation service and it works great.

But I've noticed a few areas which could be improved upon. I've described the issues I'm having with the current situation and provided a possible solution.

The three improvements that would make this package even better are:

1. Adding a configurable timeout for the failed connection/reconnect timeout
2. Refactor the console info/debug logs to an event
3. Make the AndroidRemote emit the connection close event

 

1. Configurable timeout

I'm using your package for a home automation service which has very little resources available and I would like to increase the reconnect timeout when the client receives a closed event without an error. Because I think this usually means the device is just completely off and there's no need to check it every second.

Suggestion

For example add a default timeout:

class RemoteManager extends EventEmitter {
    constructor(host, port, certs, configurableTimeout = 1000) {

RemoteManager.js

That's used in the on('close') event:

this.client.on('close', async (hasError) => {
  console.info(this.host + " Remote Connection closed ", hasError);
  if (hasError) {
    ...
  } else {
    await new Promise(resolve => setTimeout(resolve, configurableTimeout));

    await this.start().catch((error) => {
      console.error(error);
    });
  }
})

RemoteManager.js

 

2. Refactor console logs

Because the home automation service I'm using sends me the console output any time something fails it would be a huge improvement if I only get the logs which I choose to output instead of all the debug messages which are outputted to the console by this package.

Suggestion

Refactor all console.info, console.debug, console.error, etc, calls to emitting an event. I think most classes already extend the EventEmitter so you could very easily refactor the current situation:

this.client.on('timeout', () => {
    console.debug('timeout');
    this.client.destroy();
});

RemoteManager.js

To something like:

this.client.on('timeout', () => {
    this.emit('log.debug', 'timeout');
    this.client.destroy();
});

RemoteManager.js

And then make these events bubble up.

Then if you would like to keep the current behavior the only thing you would need to do is bind a listener on the client for these log events:

let androidRemote = new AndroidRemote(host, options)

androidRemote.on('log.debug', (...args) => {
    console.debug(args)
});
androidRemote.on('log.info', (...args) => {
    console.info(args)
});
androidRemote.on('log.error', (...args) => {
    console.error(args)
});

example.js

 

3. Emit the close event

The home automation service I use keeps track of if a device is available or not. There is currently no nice way for me to update this state from the events the AndroidRemote emits because it doesn't emit an event for if the connection was closed.

This would be very useful because with this event I'd be able to tell if the device is available or not. At the moment I always need to assume that the device isn't available and then update it's state to available when the AndroidRemote emits a ready event.

But there's currently no way for me to change the device's state to unavailable if the RemoteManager loses connection to the TV and starts trying to reconnect to it.

Suggestion

If you could make the close event in the RemoteManager bubble up to so the AndroidRemote emits the same event that would be a huge help!

this.client.on('close', async (hasError) => {
    this.emit('close', hasError)
    ...

RemoteManager.js

 

Let me know if there's anything I can do to help!

[vricosti]

hi, too bad you didn't try to do a PR instead.