feat: update to latest ipld-resolver #137
Conversation
|
|
||
| pull( | ||
| ipldResolver.getStream(new CID(hash)), |
There was a problem hiding this comment.
Is there no stream method on the resolver anymore Oo
There was a problem hiding this comment.
It's only available internally, to make the API match straight up what is the dag API.
src/builder/builder.js
Outdated
| node: node, | ||
| cid: new CID(node.multihash) | ||
| }, (err) => cb(err, node)) | ||
| ipldResolver.put(node, new CID(node.multihash), (err) => cb(err, node)) |
There was a problem hiding this comment.
this is much worse than before in terms of readability (the change in the resolver api)
There was a problem hiding this comment.
much worse as in 2 to 3 or 2 to 9 in a scale of 0 to 10?
There was a problem hiding this comment.
- readability before: 8
- readability after: 2
this comes from two things
- no named parameters, before the naming made clear what belonged where and as what it was used
put(node, cid, cb)is a confusing signature, asputis closest associated with something like a key value store, where you would haveput(key, value, cb). But here you first pass the node, then an id.
There was a problem hiding this comment.
It has to support two variants -- https://github.com/ipld/js-ipld-resolver#putnode-cid--format-hashalg-callback --, how would you like for this to be?
There was a problem hiding this comment.
How about
resolver.put(node, {cid: cid}, callback)
resolver.put(node, {format: 'dag-pb', hash: 'sha2-256'}, callback)
// or this could work as well, by doing an `Cid.isCid(arguments[0])` check
resolver.put(cid, node, callback)
resolver.put(node, {format: 'dag-pb', hash: 'sha2-256'}, callback)I am not a 100% sure about these, haven't had time to think through them, but they seem to be better than the current version to me.
There was a problem hiding this comment.
Given the presented concerns, I'll go with the first version, the second one is doable, but being able to have node as first and second arguments seems confusing to me.
src/exporter/file.js
Outdated
| @@ -20,7 +20,8 @@ module.exports = (node, name, ipldResolver) => { | |||
| function visitor (node) { | |||
| return pull( | |||
| pull.values(node.links), | |||
| paramap((link, cb) => ipldResolver.get(new CID(link.multihash), cb)) | |||
| paramap((link, cb) => ipldResolver.get(new CID(link.multihash), cb)), | |||
| pull.asyncMap((result, cb) => cb(null, result.value)) | |||
There was a problem hiding this comment.
why is this asyncMap, this should be just pull.map((result) => result.value))
src/exporter/index.js
Outdated
| @@ -33,7 +33,7 @@ module.exports = (hash, ipldResolver, options) => { | |||
| return pull.empty() | |||
| } | |||
| return pull( | |||
| ipldResolver.getStream(new CID(item.hash)), | |||
| ipldResolver._getStream(new CID(item.hash)), | |||
There was a problem hiding this comment.
why this breaking change on the ipldresolver api?
There was a problem hiding this comment.
nvm, saw the answer above
There was a problem hiding this comment.
There was a problem hiding this comment.
I can expose it again with the new version that also understands paths. (The internal battle between less is more or more being actually more)
There was a problem hiding this comment.
thoughts on this one?
daviddias
force-pushed
the
feat/new-ipld-resolver
branch
from
aca9f18
to
c1dadf9
Compare
|
Started seeing
|
needs: