-
Notifications
You must be signed in to change notification settings - Fork 298
fix: Support api endpoint with no port defined #744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vasco-santos please add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test
62a539b
to
52f440a
Compare
@diasdavid I have added a test for this now. All the repo tests are currently with problems, but I found the problem and already created a PR for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update ipfsd-ctl dep
52f440a
to
218e1e6
Compare
if (addrParts.length === 1 && ipRegex.v4({ exact: true }).test(addrParts[0]) && | ||
(!port || typeof port === 'object')) { | ||
// IPv4 Host with no port specified | ||
config.port = undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel as though there might be a bunch of cases here that aren't being taken into account. For example:
- I pass a host as an IP with no port and want the default port
- I pass a host as a domain name with no port and want the default port
- I pass a host as a domain name with a port
I'm not sure this is quite the right solution for the problem. If you're creating a new IpfsApi
and you don't pass a port you should expect to get the default port. In #581 the request is to pass a path as well as a host. This PR would allow the user to pass 1.2.3.4/ipfs
for their host, but this isn't a valid "host" - host being a hostname and port and might also break the default port.
I was going to suggest adding a path
or pathname
property to the config to allow people to customise the path, but I just found this (api-path
):
Can this be used with #581 and does this mitigate the need for this PR? If so, it needs to be documented and we can close out #581 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your analyze @alanshaw ! I agree with you about using api-path
for this 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rad! Would you check that it works (perhaps add a test!?) and send a PR for updating the docs?
Shall we close this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to test this later today! Then, I close this PR and create the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanshaw , I had tested this and. If I instantiate the ipfsAPI
this way:
const api = ipfsAPI({
'api-path': 'http://127.0.0.1/api/v0/',
'host': '',
'port': ''
})
The host and port default are used as well. A request example is:
http://localhost:3000http://127.0.0.1/api/v0/add?wrapWithDirectory=true&wrap-with-directory=true&stream-channels=true
Therefore, we need another property for ignoring the default config. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but for #581 I was hoping you'd be able to do this:
const api = ipfsAPI({
'api-path': '/ipfs/api/v0/',
host: '1.1.1.1',
port: 80
})
To get:
http://1.1.1.1:80/ipfs/api/v0/add?wrapWithDirectory=true&wrap-with-directory=true&stream-channels=true
...to solve the issue.
Is this the case? If so then there's no work to do here other than to send a PR to document api-path
and respond to #581.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it worked using
const api = ipfsAPI({
'api-path': '/ipfs/api/v0/',
'host': '127.0.0.1',
'port': '80'
})
I will add a test to the constructor and add the proper docs.
Thanks @alanshaw
According to my previous comments on the Issue, I submitted this PR.
The multiaddr error fallback was modified, removing the default port, in order to allow hostnames with no port specified.
Closes #581