Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

Conversation

lidel
Copy link
Member

@lidel lidel commented Aug 14, 2019

This PR:

- moves path utils to upstream utils
- closes #2333

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel requested a review from alanshaw August 14, 2019 19:18
@alanshaw
Copy link
Member

@lidel multiaddr already deals with this issue:

multiaddr('/dns4/ipfs.io/tcp/3456/https/').toString()
// outputs: /dns4/ipfs.io/tcp/3456/https

The real issue is that we want to assume "http" when converting a multiaddr to a URI but multiaddr-to-uri does not.

I think it's a reasonable assumption and I reckon we can save ourselves a bunch of trouble and boilerplate by pushing this behaviour into multiaddr-to-uri (with a configuration option to disable it).

I think we should send a PR to multiaddr-to-uri to assume http:// if /http(s) is not explicit in the multiaddr and add a configuration option to disable it. This would solve our trailing slash problem and make it easier to use multiaddr-to-uri due to less boilerplate appending /http for 99.9% of users.

So something like the following would work:

toUri('/dns4/ipfs.io/tcp/3456')
// outputs: http://ipfs.io:3456

toUri('/dns4/ipfs.io/tcp/3456/https/')
// outputs: https://ipfs.io:3456

toUri('/dns4/ipfs.io/tcp/3456', { assumeHttp: false })
// outputs: tcp://ipfs.io:3456

What do you think?

@lidel
Copy link
Member Author

lidel commented Aug 20, 2019

Makes sense. Will open PR shortly.

@alanshaw
Copy link
Member

I'm going to close this PR. Please open a new one that removes the endsWith('/http') boilerplate and updates the multiaddr-to-uri dependency when the time comes. 😘

@alanshaw alanshaw closed this Aug 20, 2019
lidel added a commit to multiformats/js-multiaddr-to-uri that referenced this pull request Aug 20, 2019
This makes multiaddrs ending with `/tcp/8080` to default to HTTP unless
an explicit assumeHttp: false is passed.

We also skip default ports for HTTP and HTTPS in the URL.

Motivation:
ipfs/js-ipfs#2358 (comment)

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member Author

lidel commented Aug 20, 2019

Continued in #2377

@alanshaw alanshaw deleted the fix/preload-with-ending-slash branch August 21, 2019 09:24
alanshaw pushed a commit to multiformats/js-multiaddr-to-uri that referenced this pull request Aug 21, 2019
This makes multiaddrs ending with `/tcp/8080` to default to HTTP unless
an explicit assumeHttp: false is passed.

We also skip default ports for HTTP and HTTPS in the URL.

Motivation:
ipfs/js-ipfs#2358 (comment)

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Https address adds incorrectly to preload addreses
2 participants