-
Notifications
You must be signed in to change notification settings - Fork 124
Conversation
SPEC/NAME.md
Outdated
@@ -76,6 +79,8 @@ This way, you can republish a new version of your website under the same address | |||
|
|||
`callback` must follow `function (err, name) {}` signature, where `err` is an error if the operation was not successful. `name` is a string that contains the IPFS hash. | |||
|
|||
`callback` must follow `function (err, result) {}` signature, where `err` is an error if the operation was not successful. `result` is an object that contains the resulting path, such as: |
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.
SPEC/NAME.md
Outdated
|
||
```JavaScript | ||
{ | ||
strings: ['/ipns/QmQrX8hka2BtNHa8N8arAq16TCVx5qHcb46c5yPewRycLm'] |
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.
"strings"? Shall we just return an array here?
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.
Yes! I got confused here. Both js-ipfs-api and js-ipfs should return only the array in the end!
js/src/name-pubsub/cancel.js
Outdated
ipfs.name.publish(value, { resolve: false }, (err, res) => { | ||
expect(err).to.not.exist() | ||
|
||
ipfs.name.resolve(nodeId, (err) => { |
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.
Just so I understand this correctly, when you resolve, you get automatically subscribed to updates? Is there a way to not subscribe?
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.
When you try to resolve
an ipns path and the experimental flag --enable-namesys-pubsub
is being used, it will get automatically subscribed. This way, when an ipns record is updated, we will have the most recent version of it stored locally.
For now, there is no way to not subscribe it (if you use the experimental flag), but since it is experimental, we may consider adding a flag for it, in case we enable it by default in the future.
js/src/name-pubsub/cancel.js
Outdated
expect(err).to.not.exist() | ||
expect(res).to.exist() | ||
expect(res).to.have.property('canceled') | ||
expect(res.canceled).to.eql(true) |
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.
Should we also check this value is no longer in our subs list?
js/src/name-pubsub/cancel.js
Outdated
expect(res.canceled).to.eql(true) | ||
|
||
done() | ||
}) |
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.
Suggestion: async waterfall or something?
js/src/name-pubsub/cancel.js
Outdated
|
||
const fixture = Object.freeze({ | ||
data: loadFixture('js/test/fixtures/testfile.txt', 'interface-ipfs-core'), | ||
cid: 'Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP' |
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.
IMHO we should just use the CID in the result from add and not hard code it. We're not testing "add" functionality here and if the CID changes in the future (because of changes in default dag layout or cid version for instance) there's fewer things to update in the tests. If you do change this then please also change in the other test files :)
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.
Will change in both
js/src/name-pubsub/subs.js
Outdated
expect(err).to.not.exist() | ||
expect(res).to.exist() | ||
expect(res).to.have.property('strings') | ||
expect(res.strings).to.eql(null) |
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.
It should always be an array.
js/src/name-pubsub/subs.js
Outdated
expect(err).to.not.exist() | ||
expect(res).to.exist() | ||
expect(res).to.have.property('strings') | ||
expect(res.strings).to.be.an('array').that.does.include(`/ipns/${nodeId}`) |
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.
Check that it was empty initially?
js/src/name-pubsub/subs.js
Outdated
expect(res.strings).to.be.an('array').that.does.include(`/ipns/${nodeId}`) | ||
|
||
done() | ||
}) |
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.
Suggestion: use async
SPEC/NAME.md
Outdated
@@ -74,7 +77,7 @@ This way, you can republish a new version of your website under the same address | |||
} | |||
``` | |||
|
|||
`callback` must follow `function (err, name) {}` signature, where `err` is an error if the operation was not successful. `name` is a string that contains the IPFS hash. | |||
`callback` must follow `function (err, result) {}` signature, where `err` is an error if the operation was not successful. `result` is an object that contains the resulting path. |
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 believe this was correct - both js-ipfs-api and js-ipfs return only the path.
1418857
to
08a5b90
Compare
PR implemented taking into consideration the most recent documentation of the
go-ipfs
API on:http://ipfs.io/ipfs/QmPKzWaeHumLuiKLb2Ak67s5kxZ1p6YbnjVKYYfF9hTjzn
This tests aim to be integrated on s-ipfs-api#846 and in a future PR to
js-ipfs
containing theIPNS over Pubsub
implementation.