search

sindresorhus / public-ip

Get your public IP address - very fast!
MIT License
1.02k stars 65 forks source link

Add redundancy, remove `https` option, add `onlyHttps` option… #34

Closed ghostnumber7 closed 4 years ago

ghostnumber7 commented 5 years ago

Some things that could be considered to be useful:

sindresorhus commented 5 years ago

DNS resolver hosts by host name not IP

Why? The IP address will not change anyway. Resolving to the IP adds a slight overhead.

Multiple DNS resolver host alternatives Multiple DNS resolver providers

Good idea. This should be documented in the readme.

Fallback to HTTPS by default if DNS does not resolve (no need to choose one or the other, just try DNS and if it fails use HTTPS)

👍

I don't really see the needs for a dns option though. I'm thinking the default behavior would be DNS + HTTPS fallback, and an option to force only HTTPS.

Added options.urls array to be able to add more urls to fallback to (like ifconfig.co)

👍

These changes will need tests and readme updates.

ghostnumber7 commented 5 years ago

NS resolver hosts by host name not IP

Why? The IP address will not change anyway. Resolving to the IP adds a slight overhead.

Don't think overhead is relevant in this case. Thought about using hostnames when I realized that I couldn't find 2620:0:ccc::2 by digging any of the opendns resolvers. They may not change, but it made me doubt xD

ghostnumber7 commented 5 years ago

I'm also thinking about randomizing the order in which dns hosts are checked to minimize the chance of going multiple times to one that's having problems before going to others ... what do you think?

ghostnumber7 commented 5 years ago

Changes in last commit:

Pending:

sindresorhus commented 5 years ago

I don't really see the needs for a dns option though. I'm thinking the default behavior would be DNS + HTTPS fallback, and an option to force only HTTPS.

⬆️

sindresorhus commented 5 years ago

Using ips instead of hostnames for DNS resolvers (probably :P)

I don't see any downside with it.

Randomizing DNS resolve order?

👍 Good idea!

Making changes on browser file to behave the same way?

👍

ghostnumber7 commented 5 years ago

So ... did your suggestions, mocked in a less verbose way (don't know if I can make it better, I normally use jest instead of ava+sinon so to be honest, I have no idea what I'm doing XD). Removed the -s option on ava CLI call, but there is no option in package.json ava config to do the same, so had to add serial to every test. Also, ava is bad at handling async stubbing, so we either have every test on its own file, or we run serially (https://github.com/avajs/ava/issues/1066) Did changes on browser file also. Did find one downside to using ipv6 ip instead of hostnames for dns resolving: When you don't have ipv6 it would normally just fail to find your ipv6 ip when resolving via host name, but when using ipv6 ip, it times out instead, and is it's making 6 checks, it takes 30 seconds for that to happen on default config. I guess it would not be a problem because you will be using it when you know you have an ipv6 enabled network. DNS option has been removed in previous commit, I updated the first comment now to reflect that. I didn't do the randomizing on the DNS order ... maybe we can add a TODO comment somewhere and have it done at some other time :P

sindresorhus commented 5 years ago

but there is no option in package.json ava config to do the same

There is. See the serial setting. You can also do it for a single file by using the import import {serial as test} from 'ava';

Also, ava is bad at handling async stubbing, so we either have every test on its own file, or we run serially (avajs/ava#1066)

The problem is neither AVA nor async, but rather concurrency. Stubbing is incompatible with concurrency.

I didn't do the randomizing on the DNS order ... maybe we can add a TODO comment somewhere and have it done at some other time :P

Can you open an issue about it? TODO comments are code rot.

sindresorhus commented 5 years ago

I don't really see the needs for a dns option though. I'm thinking the default behavior would be DNS + HTTPS fallback, and an option to force only HTTPS.

⬆️

This is not implemented. There should only be a boolean (no null) option. Maybe called onlyHttps.

ghostnumber7 commented 5 years ago

I don't really see the needs for a dns option though. I'm thinking the default behavior would be DNS + HTTPS fallback, and an option to force only HTTPS.

⬆️

This is not implemented. There should only be a boolean (no null) option. Maybe called onlyHttps.

changed to onlyHttps default false

sindresorhus commented 4 years ago

Did you see https://github.com/sindresorhus/public-ip/issues/40? I think we should remove icanhazip.com. But that leaves us with only ipify.org for the browser. Do you know any other free IP service that works over HTTPS?

sindresorhus commented 4 years ago

Very nice! Thanks for working on this 🙌