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: add default port for protocol #3

Merged
merged 4 commits into from
Jan 29, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

[![Build Status](https://travis-ci.org/tableflip/uri-to-multiaddr.svg?branch=master)](https://travis-ci.org/tableflip/uri-to-multiaddr) [![dependencies Status](https://david-dm.org/tableflip/uri-to-multiaddr/status.svg)](https://david-dm.org/tableflip/uri-to-multiaddr) [![JavaScript Style Guide](https://img.shields.io/badge/code_style-standard-brightgreen.svg)](https://standardjs.com)

> Convert a URI to a [Multiaddr](https://multiformats.io/multiaddr/): https://protocol.ai -> /dns4/protocol.ai/https
> Convert a URI to a [Multiaddr](https://multiformats.io/multiaddr/): https://protocol.ai -> /dns4/protocol.ai/tcp/80/https
olizilla marked this conversation as resolved.
Show resolved Hide resolved

## Install

Expand All @@ -16,7 +16,7 @@ npm install uri-to-multiaddr
const toMultiaddr = require('uri-to-multiaddr')

console.log(toMultiaddr('https://protocol.ai'))
// -> /dns4/protocol.ai/https
// -> /dns4/protocol.ai/tcp/443/https
```

Domain names can represent one of
Expand All @@ -32,15 +32,15 @@ in an options object as the second parameter to override it:
```js
const toMultiaddr = require('uri-to-multiaddr')

console.log(toMultiaddr('https://protocol.ai'), {defaultDnsType: 'dnsaddr'})
// -> /dnsaddr/protocol.ai/https
console.log(toMultiaddr('https://protocol.ai'), { defaultDnsType: 'dns6' })
// -> /dns6/protocol.ai/tcp/443/https
```

See [test.js](./test.js) for the currently supported conversions.

**Note**: `uri-to-multiaddr` will throw if the passed URI:
- is not a valid, according the WHATWG URL spec implementation used.
- is not supported yet e.g. quic
- is not supported yet

## Related

Expand Down
37 changes: 24 additions & 13 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
const url = require('url')
const isIp = require('is-ip')
const Multiaddr = require('multiaddr')

/**
* Convert a URI to a multiaddr
*
* http://foobar.com => /dns4/foobar.com/https
* https://foobar.com:8080 => /dns4/foobar.com/tcp/8080/https
* ws://foobar.com => /dns4/foobar.com/ws
* https://127.0.0.1:8080 => /ip4/127.0.0.1/tcp/8080/https
* http://[::1]:8080 => /ip6/::1/tcp/8080/http
* http://foobar.com => /dns4/foobar.com/tcp/80/http
* https://foobar.com => /dns4/foobar.com/tcp/443/tls/http
olizilla marked this conversation as resolved.
Show resolved Hide resolved
* https://foobar.com:5001 => /dns4/foobar.com/tcp/5001/tls/http
olizilla marked this conversation as resolved.
Show resolved Hide resolved
* https://127.0.0.1:8080 => /ip4/127.0.0.1/tcp/8080/tls/http
olizilla marked this conversation as resolved.
Show resolved Hide resolved
* http://[::0]:8080 => /ip6/::0/tcp/8080/http
Copy link
Member

Choose a reason for hiding this comment

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

Why tho 🤔 For ip4 (1 line above) we default to localhost, shouldn't we do the same here? (::1)

I know this is a nit, but there is a huge difference if a beginner just copies&pastes 0.0.0.0:5001 vs 127.0.0.1:5001 without thinking, so I always try to avoid "listen on all interfaces" in examples :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

olizilla marked this conversation as resolved.
Show resolved Hide resolved
* tcp://foobar.com:8080 => /dns4/foobar.com/tcp/8080
* udp://foobar.com:8080 => /dns4/foobar.com/udp/8080
*/
Expand All @@ -33,15 +32,16 @@ function multiaddrFromUri (uriStr, opts) {

function parseUri (uriStr) {
// Use the WHATWG URL global, in node >= 10 and the browser
const { protocol, hostname } = new URL(uriStr)
// WHATWG URL hides port when it's the default for the scheme...
const port = url.parse(uriStr).port
let { protocol, hostname, port } = new URL(uriStr)
const scheme = protocol.slice(0, -1)
if (!port && port !== 0) {
port = portForProtocol(scheme)
}
return { scheme, hostname, port }
}

function tupleForHostname (hostname, defaultDnsType) {
if (!hostname) throw new Error('hostname is requried')
if (!hostname) return null
if (isIp.v4(hostname)) {
return ['ip4', hostname]
}
Expand Down Expand Up @@ -69,10 +69,21 @@ function tupleForPort (port, scheme) {
}

function tupleForScheme (scheme) {
if (scheme.match(/^https?$|^wss?$/)) {
return [scheme]
if (scheme.match(/^tcp$|^udp$/)) {
return null
}
return [scheme]
}

function portForProtocol (protocol) {
if (!protocol) return null
const portFor = {
http: '80',
https: '443',
ws: '80',
wss: '443'
}
return null
return portFor[protocol]
}

module.exports = multiaddrFromUri
77 changes: 9 additions & 68 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"dependencies": {
"ava": "^0.25.0",
"is-ip": "^2.0.0",
"multiaddr": "^5.0.0"
"multiaddr": "^6.0.3"
},
"devDependencies": {
"standard": "^12.0.1"
Expand Down
34 changes: 24 additions & 10 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ const toMultiaddr = require('./')

test('should convert URIs to multiaddrs', (t) => {
const data = [
['/ip4/127.0.0.1/http', 'http://127.0.0.1'],
['/ip6/fc00::/http', 'http://[fc00::]'],
['/ip4/127.0.0.1/tcp/80/http', 'http://127.0.0.1'],
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should keep tests for both versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean ip4 and ip6? Both tests have been kept, I just updated to include the port. Pesky diff.

['/ip6/fc00::/tcp/80/http', 'http://[fc00::]'],
['/ip4/0.0.7.6/tcp/1234', 'tcp://0.0.7.6:1234'],
['/ip4/0.0.7.6/tcp/1234/http', 'http://0.0.7.6:1234'],
['/ip4/0.0.7.6/tcp/1234/https', 'https://0.0.7.6:1234'],
Expand All @@ -14,17 +14,22 @@ test('should convert URIs to multiaddrs', (t) => {
['/dns4/protocol.ai/tcp/80', 'tcp://protocol.ai:80'],
['/dns4/protocol.ai/tcp/80/http', 'http://protocol.ai:80'],
['/dns4/protocol.ai/tcp/80/https', 'https://protocol.ai:80'],
['/dns4/ipfs.io/ws', 'ws://ipfs.io'],
['/dns4/ipfs.io/http', 'http://ipfs.io'],
['/dns4/ipfs.io/https', 'https://ipfs.io'],
['/dns4/ipfs.io/tcp/80/ws', 'ws://ipfs.io'],
['/dns4/ipfs.io/tcp/443/wss', 'wss://ipfs.io'],
['/dns4/ipfs.io/tcp/80/http', 'http://ipfs.io'],
['/dns4/ipfs.io/tcp/443/https', 'https://ipfs.io'],
['/ip4/1.2.3.4/tcp/3456/ws', 'ws://1.2.3.4:3456'],
['/ip4/1.2.3.4/tcp/3456/wss', 'wss://1.2.3.4:3456'],
['/ip6/::/tcp/0/ws', 'ws://[::]:0'],
['/dns4/ipfs.io/wss', 'wss://ipfs.io'],
['/ip4/1.2.3.4/tcp/3456/wss', 'wss://1.2.3.4:3456'],
['/ip6/::/tcp/0/wss', 'wss://[::]:0']
]

data.forEach(d => t.is(toMultiaddr(d[1]).toString(), d[0], `Converts ${d[1]} to ${d[0]}`))
data.forEach(d => {
const input = d[1]
const expected = d[0]
const output = toMultiaddr(input).toString()
t.is(output, expected, `Converts ${input} to ${expected}`)
})
})

test('should use the defaultDnsType where provided', (t) => {
Expand All @@ -37,6 +42,15 @@ test('should use the defaultDnsType where provided', (t) => {
data.forEach(d => t.is(toMultiaddr(d[1], d[2]).toString(), d[0], `Converts ${d[1]} to ${d[0]} with opts ${d[2]}`))
})

test('should throw for unsupported protocol', (t) => {
t.throws(() => toMultiaddr('quic://'))
test('should throw for on invalid url', (t) => {
t.throws(() => {
toMultiaddr('whoosh.fast')
}, /Invalid URL/)
})

test('should throw for unknown protocol', (t) => {
t.throws(() => {
// NOTE: `data` is a valid uri protocl but isn't a valid multiaddr protocol yet
olizilla marked this conversation as resolved.
Show resolved Hide resolved
toMultiaddr('data:image/svg+xml;base64,test')
}, 'no protocol with name: data')
})