feat: add support to resolve dns to ipns #458
Conversation
ghost
assigned 
| @@ -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) => { | |||
There was a problem hiding this comment.
Camel case please - allowOffline.
There was a problem hiding this comment.
camel case doesn't work alan, this is used directly as http payload for go-ipfs
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@alanshaw can we defer this to another PR ? ill make an issue to track
There was a problem hiding this comment.
Sure, but we should get it resolved before the next release as it's a breaking change to a brand new feature otherwise.
There was a problem hiding this comment.
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.
|
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
hugomrdias
force-pushed
the
feat/name-resolve-dns
branch
from
f51d747
to
64924ae
Compare
| } | ||
| it('should resolve /ipns/ipfs.io', async () => { | ||
| return expect(await ipfs.name.resolve('/ipns/ipfs.io')) | ||
| .to.match(/\/ipfs\/.+$/) |
There was a problem hiding this comment.
we cant for the remainder tests i would prefer to keep them all the same
|
Create an issue for the ping problem in js-ipfs |
This PR also normalizes
ipns name resolvereturn value and offline options, to integrate better with http client and ipfs core.