Refactor ipld to use pull streams for get #122
Conversation
There was a problem hiding this comment.
Wow great work! Somehow I overlooked this PR. Feel free to ping me next time that happens. I always try to do code reviews in a timely manner.
I've only minor comments that don't really need changes. I see code review as a chance to also learn about different coding styles/how others would do it, so I left comments in that regard.
As returning the result as an array is a breaking change, I'd like to get a @diasdavid view on it. For me it sounds good.
src/index.js
Outdated
|
|
||
| pull(this.getPullStream(cid, path, options), | ||
| pull.reduce((arr, item) => { | ||
| if (!options.onlyNode) { |
There was a problem hiding this comment.
This is just a general comment, nothing you need to change: Often code is easier to understand without negations. I would flip this if statement to:
if(options.onlyNode) {
// replace existing entry
arr[0] = item
} else {
arr.push(item)
}
I might also change the comment to something like "the only item is the last one" or "reducing to the last item".
| if (err || abort) { | ||
| return cb(err) | ||
| } | ||
| const r = this.resolvers[cid.codec] |
There was a problem hiding this comment.
A nice trick I learnt a few years ago from a colleague. Single letter variable names are really hard to grep for, but are useful. To keep the terseness but make it easier to grep, just double the single. So this becomes const rr = this.resolvers[cid.codec].
src/index.js
Outdated
| callback(null, { | ||
| value: node, | ||
| remainderPath: '' | ||
| return function read (abort, cb) { |
There was a problem hiding this comment.
I tend to prefix variables that aren't used with an underscore. This way you don't wonder if abort wasn't used accidentally or intentionally. Though I'm not sure if that's done that way in this codebase yet.
test/ipld-all.js
Outdated
| @@ -62,7 +62,8 @@ describe('IPLD Resolver for dag-cbor + dag-pb', () => { | |||
| it('resolve through different formats', (done) => { | |||
| resolver.get(cidCbor, 'pb/Data', (err, result) => { | |||
| expect(err).to.not.exist() | |||
| expect(result.value).to.eql(Buffer.from('I am inside a Protobuf')) | |||
| expect(result.length).to.eq(2) | |||
| expect(result[1].value).to.eql(Buffer.from('I am inside a Protobuf')) | |||
There was a problem hiding this comment.
I think there should be a test the also tests all results, not just the last one. Though it might make sense to make a dedicated test for it, so that the other tests are kept short to the point.
Fine by me. It makes it consistent with the IPFS APIs for when there is more than a way to return (array vs pull vs readable) |
|
@zcstarr The other projects that would need to be updated are: |
|
@vmx thanks for the feedback! mind taking another look ? |
src/index.js
Outdated
| if (options.onlyNode) { | ||
| arr[0] = item | ||
| } else { | ||
| // reducing to the last item |
There was a problem hiding this comment.
That comment is wrong, it should be on the if case.
test/ipld-dag-cbor.js
Outdated
| @@ -115,10 +115,11 @@ module.exports = (repo) => { | |||
| }) | |||
|
|
|||
| it('resolver.get just CID', (done) => { | |||
| resolver.get(cid1, (err, result) => { | |||
| resolver.get(cid1, (err, results) => { | |||
There was a problem hiding this comment.
In all other tests you didn't rename result to results. This is just a something I'd like to mention in case you care a lot about consistency. I'm happy to leave it like that (there's no point of wasting time on such small things, but sometimes I do then though ;)
|
@vmx quick question is the expectation that js-ipfs ipfs.dag.get returns array as well, or should I make the integration of this pull stream PR a non breaking change with js-ipfs ? |
|
@zcstarr A non-breaking change sounds good. I would then start a discussion about the breaking change later on. @diasdavid Do you agree? |
|
Yeah. Let's have .get and .getPullStream both in js-ipfs and js-ipld just like the Files API does -- https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md. No need to implement the ReadableStream endpoints at this level. |
vmx
added
status/deferred
|
@zcstarr Thanks for all your work on this. A lot has happened since this PR was originally opened. We don't use pull-streams anymore, but async iterators. Hence I'm closing this issue. |
ghost
removed
the
This a PR to refactor ipld resolver to use pull streams #101 , as well as change the default behavior from returning a single value in a callback to returning an array. In addition to this the changes will expose getPullStream which should allow people to read the stream and process each node along the path.
In this PR, I didn't add explicit tests for proper path resolution (i.e. the collected along the path are correct), wanted to get a little more feedback first. Also note this will be a breaking change.
@vmx PTAL when you get a chance, and let me know if the interface should change or any thing you'd like to see implemented differently, also if you have any pointers to other projects that will need to be updated to support this change, that would be great!