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

ghostnumber7 · 2019-08-25T21:41:04Z

Some things that could be considered to be useful:

Multiple DNS resolver host alternatives
Multiple DNS resolver providers
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)
Ability to disable or enable HTTPS check ({ https: true } will only check HTTPS). Defaults to DNS + HTTPS
Added options.urls array to be able to add more urls to fallback to (like ifconfig.co)

sindresorhus · 2019-08-26T00:14:58Z

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.

index.js

sindresorhus · 2019-08-26T00:16:33Z

index.js

+		data.dnsServers.forEach(dnsServerInfo => {
+			const {servers, question} = dnsServerInfo;
+			servers.forEach(server => {
+				// eslint-disable-next-line promise/prefer-await-to-then


The rule is there for a reason. There's never a case where you have to use .then.

I tried to do a for-in loop with await but linter was not happy with it ... in this case, I chain then after then so that they run concurrently to simulate a for loop with await inside ... maybe there's a better approach I'm not seeing

index.js

ghostnumber7 · 2019-08-26T00:39:28Z

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 · 2019-08-26T00:51:46Z

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 · 2019-08-26T15:22:30Z

Changes in last commit:

removed forEach in favour of for-of
added tests for new features
added stubs to control flow failures
documented things (I think my documentation skills are terrible so feel free to improve)

Pending:

Using ips instead of hostnames for DNS resolvers (probably :P)
Randomizing DNS resolve order?
Making changes on browser file to behave the same way?

sindresorhus · 2019-09-13T12:03:12Z

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 · 2019-09-13T12:03:56Z

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?

👍

sindresorhus · 2019-09-13T11:55:01Z

package.json

@@ -13,7 +13,7 @@
 		"node": ">=8"
 	},
 	"scripts": {
-		"test": "xo && ava test.js && tsd"
+		"test": "xo && ava -s test.js && tsd"


Use the AVA package.json config instead of the CLI flag.

sindresorhus · 2019-09-13T11:55:46Z

readme.md

+
+##### urls
+
+Type: `array`<br>


Suggested change

Type: `array`<br>

Type: `string[]`<br>

readme.md

test.js

mocks/dns-socket.js

sindresorhus · 2019-09-13T11:58:44Z

mocks/got.js

+	ignoreRegExp = undefined;
+	ignored = [];
+	called = 0;
+};


Is there really no less verbose way to do the mocking?

ghostnumber7 · 2019-09-15T17:55:47Z

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 (avajs/ava#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 · 2019-09-29T18:21:44Z

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 · 2019-10-02T10:57:05Z

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.

index.js

test.js

readme.md

ghostnumber7 · 2019-10-23T20:31:05Z

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 · 2019-11-15T11:25:50Z

Did you see #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?

index.d.ts

readme.md

test.js

mocks/stub.js

browser.js

readme.md

sindresorhus · 2019-11-22T06:53:39Z

Very nice! Thanks for working on this 🙌

Multiple DNS and HTTPS retries & HTTPS fallback when DNS fails

6cd24ea

sindresorhus requested changes Aug 26, 2019

View reviewed changes

Improvements, tests and docs.

e17b83b

ghostnumber7 requested a review from sindresorhus September 12, 2019 13:41

sindresorhus requested changes Sep 13, 2019

View reviewed changes

suggested changes

7ff31cb

ghostnumber7 requested a review from sindresorhus September 15, 2019 17:55

sindresorhus requested changes Oct 2, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

test.js Outdated Show resolved Hide resolved

test.js Outdated Show resolved Hide resolved

readme.md Outdated Show resolved Hide resolved

requested changes

7330e23

fix missing param

ac103b8

ghostnumber7 requested a review from sindresorhus October 23, 2019 20:33

sindresorhus added 3 commits November 15, 2019 18:31

Update dns-socket.js

85438d6

Update got.js

49a15cf

Update stub.js

0c0b547

sindresorhus requested changes Nov 15, 2019

View reviewed changes

ghostnumber7 added 4 commits November 17, 2019 21:07

partial comments

42c382b

Merge branch 'master' of github.com:ghostnumber7/public-ip

9930e00

travis ipv6 test

36e1d27

test travis

75ba58b

Update .travis.yml

b9f82a4

sindresorhus reviewed Nov 18, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

ghostnumber7 added 3 commits November 18, 2019 10:12

remove ipv6 travis test

cea7aad

fix

9eb56cc

text fixes

80bee8f

ghostnumber7 requested a review from sindresorhus November 18, 2019 17:55

sindresorhus changed the title ~~Multiple DNS and HTTPS retries & HTTPS fallback when DNS fails~~ Add redundancy, remove https option, add onlyHttps option, add fallbackUrls option Nov 22, 2019

sindresorhus changed the title ~~Add redundancy, remove https option, add onlyHttps option, add fallbackUrls option~~ Add redundancy, remove https option, add onlyHttps option… Nov 22, 2019

sindresorhus merged commit fdcbb7d into sindresorhus:master Nov 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghostnumber7 commented Aug 25, 2019 •

edited

Loading

sindresorhus commented Aug 26, 2019

sindresorhus Aug 26, 2019

ghostnumber7 Aug 26, 2019

ghostnumber7 commented Aug 26, 2019

ghostnumber7 commented Aug 26, 2019 •

edited

Loading

ghostnumber7 commented Aug 26, 2019 •

edited

Loading

sindresorhus commented Sep 13, 2019

sindresorhus commented Sep 13, 2019

sindresorhus Sep 13, 2019

sindresorhus Sep 13, 2019

sindresorhus Sep 13, 2019

ghostnumber7 commented Sep 15, 2019

sindresorhus commented Sep 29, 2019

sindresorhus commented Oct 2, 2019

ghostnumber7 commented Oct 23, 2019

sindresorhus commented Nov 15, 2019

sindresorhus commented Nov 22, 2019

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

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

Conversation

ghostnumber7 commented Aug 25, 2019 • edited Loading

sindresorhus commented Aug 26, 2019

sindresorhus Aug 26, 2019

Choose a reason for hiding this comment

ghostnumber7 Aug 26, 2019

Choose a reason for hiding this comment

ghostnumber7 commented Aug 26, 2019

ghostnumber7 commented Aug 26, 2019 • edited Loading

ghostnumber7 commented Aug 26, 2019 • edited Loading

sindresorhus commented Sep 13, 2019

sindresorhus commented Sep 13, 2019

sindresorhus Sep 13, 2019

Choose a reason for hiding this comment

sindresorhus Sep 13, 2019

Choose a reason for hiding this comment

sindresorhus Sep 13, 2019

Choose a reason for hiding this comment

ghostnumber7 commented Sep 15, 2019

sindresorhus commented Sep 29, 2019

sindresorhus commented Oct 2, 2019

ghostnumber7 commented Oct 23, 2019

sindresorhus commented Nov 15, 2019

sindresorhus commented Nov 22, 2019

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

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

ghostnumber7 commented Aug 25, 2019 •

edited

Loading

ghostnumber7 commented Aug 26, 2019 •

edited

Loading

ghostnumber7 commented Aug 26, 2019 •

edited

Loading