Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: assume dns4 for domain names #2

Merged
merged 2 commits into from
Jan 25, 2019
Merged

fix: assume dns4 for domain names #2

merged 2 commits into from
Jan 25, 2019

Conversation

olizilla
Copy link
Contributor

  • switch the defualt protocol for domain names from dnsaddr to dns4
  • provide an override to let the caller pick the default protocol for domains

A better fix for this will be possible once multiformats/multiaddr#22 is decided.

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

- switch the defualt protocol for domain names from dnsaddr to dns4
- provide an override to let the caller pick the default protocol for domains

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library assumes /dns4 when it finds a domain name in the input string. It makes no attempt query DNS. To override the default assumption, you can pass in an options object as the second parameter to override it

This should be a safe default given current reality that ipv6 websites are a subset of ipv4 ones, and the library will be primarily used in frontend places like ipfs-webui, where dns resolution is handled by the browser anyway.

cc @lgierth as he is familiar with historical decisions related to /dns4, /dns6 and /dnsaddr

index.js Outdated Show resolved Hide resolved
['/dnsaddr/ipfs.io/ws', 'ws://ipfs.io'],
['/dnsaddr/ipfs.io/http', 'http://ipfs.io'],
['/dnsaddr/ipfs.io/https', 'https://ipfs.io'],
['/dns4/protocol.ai/tcp/80', 'tcp://protocol.ai:80'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add the same /dns6/ entries as well, just for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidel there is a seperaste test for the defaultDnsType below, is that what you were looking for?

Co-Authored-By: olizilla <oli@tableflip.io>
@olizilla
Copy link
Contributor Author

looking at it now, I think the current defacto way of expressing https is to nest http in tls...

@lidel
Copy link
Member

lidel commented Jan 25, 2019

@olizilla see my note about /tls/http vs /https at #3 (comment)

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

Successfully merging this pull request may close these issues.

3 participants