feat: create car file from a complete graph #3
Conversation
|
Seems fine to me. Just thinking through the ways in which this is likely to expand so we don't end up stepping on our own toes. There are two: multiple roots and selectors. As comparison with go-car that can now handle both situations with traversal, the new Not necessarily a great API to follow but not too far away from what you have now. If we put |
car.js
Outdated
| async function traverseBlock (block, get, car, seen = new Set()) { | ||
| const cid = await block.cid() | ||
| await car.put(cid, block.encodeUnsafe()) | ||
| seen.add(cid.toString('base64')) |
There was a problem hiding this comment.
doesn't work with CIDv0 unfortunately, maybe just a toString() here and line 144?
There was a problem hiding this comment.
so, the way CID.toString() works is that it caches the string type it was instantiated with and uses that as the default. this was done for perf and for some backwards compatibility, but it makes it problematic when using toString() as a cache key because it isn’t guaranteed to be consistent 😕
There was a problem hiding this comment.
So what do? ipfs/js-ipfs#2953 doesn't actually work because of this, I'm using your branch but manually edited this fie to remove 'base64' here and below, otherwise it blows up as soon as it encounters a CIDv0. Do we need to if/else on the version?
There was a problem hiding this comment.
We can just switch to explicit ‘base58’ instead. I can’t think of anything it would break.
|
I just pushed a fix. I had forgotten that |
car.js
Outdated
| } | ||
| } | ||
|
|
||
| async function completeGraph (root, get, car) { |
There was a problem hiding this comment.
thoughts on moving root to last so we can ...root at some point?
There was a problem hiding this comment.
i ended up having to add an optional parameter for configurable concurrency.
car.js
Outdated
| seen.add(cid.toString('base58btc')) | ||
| if (cid.codec === 'raw') return | ||
| const reader = block.reader() | ||
| const missing = link => !seen.has(link.toString('base58btc')) |
There was a problem hiding this comment.
One minor style nit that grates on me is arrow functions without parens. (link) => ... to make it crystal clear what's going on here
rvagg
force-pushed
the
complete-graph
branch
2 times, most recently
from
04ded7b to
6ce7912
Compare
|
added docs, updated deps and fixed up coverage, made some minor style tweaks. This is ready to land as long as I can get Actions to work on it. |
I tried to match the style but I’m sure there’s some differences you may want me to clean up before merging.
I’m not sure if this is your preferred API, it matches what we have in a lot of the other JS libraries but isn’t exactly aligned with the API’s that are here currently. I figured I should wait to write up the docs until you had a chance to look at the PR and potentially change the API.