Skip to content
This repository has been archived by the owner on Apr 29, 2018. It is now read-only.

Unable to unset a host once configured #20

Open
orderoftheflame opened this issue Mar 12, 2018 · 1 comment
Open

Unable to unset a host once configured #20

orderoftheflame opened this issue Mar 12, 2018 · 1 comment

Comments

@orderoftheflame
Copy link

orderoftheflame commented Mar 12, 2018

Hi.

I've run into a problem where once a host has been set (in the config object passed to the eiscp.connect function), it is impossible to unset this, meaning that you cannot revert to selecting the first found receiver on the network.

It looks like line 290 in eiscp.js is preventing this from ever being cleared:

config.host = options.host || config.host;

Meaning that line 303 which determines whether or not to pick the first found device always resolves as false:

if (typeof config.host === 'undefined' || config.host === '') {

While I can see it as useful for the other options to not be unset, to clear the host and revert back to discovering the first host would be useful.

As an example.

var eiscp = require('eiscp');

 var connectionOptions = { port: 60128, reconnect: false, reconnect_sleep: 5, modelsets: [], send_delay: 500, verify_commands: false };

// connectionOptions.host is undefined
 eiscp.connect(connectionOptions);
 //Connects fine
 eiscp.disconnect();

connectionOptions.host = '192.168.1.81';
 eiscp.connect(connectionOptions);
 //Connects fine
 eiscp.disconnect();

connectionOptions.host = '';
 eiscp.connect(connectionOptions);
 //Attempts to connect to 192.168.1.81 regardless of other receivers.
 eiscp.disconnect();

connectionOptions.host = undefined;
 eiscp.connect(connectionOptions);
 //Attempts to connect to 192.168.1.81 regardless of other receivers.
 eiscp.disconnect();
orderoftheflame added a commit to orderoftheflame/node-eiscp that referenced this issue Mar 12, 2018
@orderoftheflame
Copy link
Author

orderoftheflame commented Mar 12, 2018

I need this change for a project I'm working on so I've forked and made the change for my own purposes, I'm not sure if this requirement is an issue or by design though.

master...orderoftheflame:master

I'm happy to create a pull request if this seems okay though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant