Add ip-ranges to find()

DylanPiercey / local-devices

🔮 Find devices connected to the current local network.

MIT License

140 stars 27 forks source link

Add ip-ranges to find() #24

Closed Miosame closed 4 years ago

Miosame commented 4 years ago

I'm not sure if the way the servers array is filled aligns with the general idea and what should happen if a wrong range is passed, so that's up to discussion / review process.

Currently it:

checks inside findLocalDevices() if it is a range and assigns / fills the servers array based on that.
returns the error, which should probably be made into a throw instead (or an empty return to return undefined, similar to how it is done throughout other parts of the code?) together with notes inside the readme if it will throw instead of returning undefined.

Again: Edits are checked / allowed and welcome.

Miosame commented 4 years ago

Thanks! will take a look at the rest once I come home later tonight.

Miosame commented 4 years ago

Done, implemented the suggestions and cleaned up a little.

DylanPiercey commented 4 years ago

It would be nice if when a range is used that it only ping'd servers in that range. Currently the most costly part of this module is pinging all of the servers and ranges could help with that.

Miosame commented 4 years ago

@DylanPiercey that already should be taken care of by:

https://github.com/Miosame/local-devices/blob/1d642f9a140bdff79e831f97bd1fedc83390074e/src/index.js#L27-L37

and

https://github.com/Miosame/local-devices/blob/1d642f9a140bdff79e831f97bd1fedc83390074e/src/index.js#L77-L82

I believe?

DylanPiercey commented 4 years ago

@Miosame ah yes, sorry I missed that.

Miosame commented 4 years ago

@DylanPiercey no worries, I had to check too first 👍

Miosame commented 4 years ago

@DylanPiercey I'm currently also under heavy load, but just remembered about this - is there anything that worries you before the merge we could look at?

Miosame commented 4 years ago

@DylanPiercey bump? :tada:

DylanPiercey commented 4 years ago

@Miosame could you add an extra example to the docs demonstrating the range functionality? I think it looks good to merge otherwise.

natterstefan commented 4 years ago

Good point @DylanPiercey. Can you take care of this @Miosame, please?

Miosame commented 4 years ago

@natterstefan @DylanPiercey sure will do this later today or tomorrow :+1:

Miosame commented 4 years ago

@natterstefan @DylanPiercey done :tada: (not sure if I should add the contributor emojis though)

Miosame commented 4 years ago

@natterstefan didn't notice that locally, thanks!