Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

feat: add support to resolve dns to ipns #458

Merged
merged 12 commits into from
Jun 20, 2019
Merged

Conversation

hugomrdias
Copy link
Contributor

This PR also normalizes ipns name resolve return value and offline options, to integrate better with http client and ipfs core.

src/name/resolve.js Outdated Show resolved Hide resolved
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Great! Except for a minor detail, this looks good!

src/name/resolve.js Outdated Show resolved Hide resolved
@@ -43,7 +43,7 @@ module.exports = (createCommon, options) => {

const value = fixture.cid

ipfs.name.publish(value, (err, res) => {
ipfs.name.publish(value, { 'allow-offline': true }, (err, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Camel case please - allowOffline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

camel case doesn't work alan, this is used directly as http payload for go-ipfs

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be - core takes camel case options, if it needs to be converted to dash-case for the query string then js-ipfs-http-client needs to convert it and js-ipfs HTTP endpoint needs to convert it back. This is consistent with many of the other endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanshaw can we defer this to another PR ? ill make an issue to track

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but we should get it resolved before the next release as it's a breaking change to a brand new feature otherwise.

src/name/resolve.js Outdated Show resolved Hide resolved
src/name/resolve.js Show resolved Hide resolved
Copy link
Contributor

@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.

name.resolve command proved to be a tricky code path in past and it is extremely important in web browser contexts. Everything breaks if it does not work as expected, so I'd be super thorough with tests 🔍

What if we had a matrix of tests where resolve of ID/hostname/full path is tested against 3 states of recursive parameter?

Something like this (:question: indicates missing test in 97eb43, I hope I got it right):

value to name.resolve recursive=(default) recursive=true recursive=false
${nodeId} 🍏 🍏
ipfs.io 🍏 🍏
/ipns/${nodeId}/remainder/file.txt 🍏
/ipns/ipfs.io/images/ipfs-logo.svg 🍏 🍏

This would detect any regressions/discrepancies including the default behavior without custom recursive option (which recently changed in go-ipfs as noted in this comment and ipfs/js-ipfs#2001).

There should also be at least one test of recursive lookup that is looking into a sharded directory (details below), but that should not block this, can be added in a separate PR.

src/name/resolve.js Outdated Show resolved Hide resolved
@alanshaw
Copy link
Contributor

alanshaw commented May 9, 2019

Is this ready for a re-review @hugomrdias or are there more commits you want to add? 😁

This PR also normalizes `ipns name resolve` return value and offline options, to integrate better with http client and ipfs core.
'should resolve a DNS link' is a duplicate plus the `r: true` isn't documented

'should non-recursively resolve ipfs.io' doesn't need to be skiped anymore
}
it('should resolve /ipns/ipfs.io', async () => {
return expect(await ipfs.name.resolve('/ipns/ipfs.io'))
.to.match(/\/ipfs\/.+$/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use is-ipfs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cant for the remainder tests i would prefer to keep them all the same

src/ping/ping-pull-stream.js Outdated Show resolved Hide resolved
src/ping/ping-readable-stream.js Outdated Show resolved Hide resolved
src/ping/ping.js Outdated Show resolved Hide resolved
@hugomrdias
Copy link
Contributor Author

Create an issue for the ping problem in js-ipfs
ipfs/js-ipfs#2186

@hugomrdias hugomrdias requested a review from alanshaw June 19, 2019 21:21
@alanshaw alanshaw merged commit cd41a3c into master Jun 20, 2019
@alanshaw alanshaw deleted the feat/name-resolve-dns branch June 20, 2019 06:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants