feat: new IPLD Format API #127
Conversation
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
==========================================
+ Coverage 91.14% 94.11% +2.97%
==========================================
Files 14 15 +1
Lines 271 238 -33
==========================================
- Hits 247 224 -23
+ Misses 24 14 -10
Continue to review full report at Codecov.
|
src/dag-node/create.js
Outdated
| } | ||
|
|
||
| return callback(null, new DAGNode(data, links, buffer.length)) | ||
| const serialized = await serialize({ |
There was a problem hiding this comment.
serialize() doesn't return a Promise at all so await can be removed from this
src/resolver.js
Outdated
| remainderPath: split.slice(1).join('/') | ||
| }) | ||
| } | ||
| exports.resolve = async (binaryBlob, path) => { |
There was a problem hiding this comment.
async can be removed from this, it'll just make an unnecessary Promise
There was a problem hiding this comment.
It's for consistency with other formats. I'd like to keep it for now until it turns out to be an issue.
There was a problem hiding this comment.
I get the consistency argument but really think the format API needs to be "MaybePromise" rather than "Promise", or just make resolve() completely synchronous. None of the formats do anything async within resolve(), they're all the same pattern as this one (other than raw which is even simpler):
- https://github.com/ipld/js-ipld-dag-cbor/pull/107/files#diff-fe783c782beb75b01586f9afb052da1bR21
- https://github.com/ipld/js-ipld-zcash/pull/39/files#diff-fe783c782beb75b01586f9afb052da1bR21
- https://github.com/ipld/js-ipld-bitcoin/pull/48/files#diff-fe783c782beb75b01586f9afb052da1bR21
- https://github.com/ipld/js-ipld-git/pull/51/files#diff-fe783c782beb75b01586f9afb052da1bR21
- https://github.com/ipld/js-ipld-raw/pull/32/files#diff-1fdf421c05c1140f6d71444ea2b27638R22
So the async here is theoretical at best. I like that it might be accommodating a potential of encryption or something else novel, but so far it doesn't, and that could be accomplished by stating in the format specification that it "may be a Promise" so users may want to opt for await if they want to be maximally compatible. I suspect most uses of the current code are sticking to specific implementations though and that flexibility can be left to the next generation stack.
As I understand, Matteo's performance analysis identified as the faux-async with setImmediate() everywhere was the biggest cost for js-ipfs. Replacing them with async doesn't buy much at all, it's just different sugar for deferring something that isn't actually asynchronous.
There was a problem hiding this comment.
I'll make resolve() sync for the same reason that I made serialize()/deserialize() sync. Thanks for this outside perspective. I'm currently too deep into this without seeing the bigger picture.
test/dag-node-test.js
Outdated
| }) | ||
|
|
||
| it('dagNode.toJSON with data no links', (done) => { | ||
| it('dagNode.toJSON with data no links', async () => { |
|
Sorry, there's a bunch of noise in there. First, the Title cased properties aren't great. Title case in idiomatic JavaScript should be reserved for class / prototype / global object names, elsewhere it's just out of place. Next, the async thing is really interesting. This is the only truly set of async functions in the whole lot:
And it's only in One nice thing about So, in the case of ipld-dag-pb, the only thing we're doing that is async is some It'd be pretty neat to ditch all of these pointless |
|
OK, I see where the Titlecase is coming from, it's in the PB data model ipld/specs#114 (and util.js). I still don't think that should leak out to the JS interface though, it should be the job of the interface to translate a data layer into a language-idiomatic presentation of it. |
|
I've opened an issue for further |
|
I also don't like those Camelcase property names. But it makes sense in regards how current IPLD Formats are envisioned. If they serialize/deserialize synchronously then |
|
@rvagg For future reference, feel free to mention things like " |
|
My remaining hangup is with |
|
And README needs updating, is that a separate aegir step? |
BREAKING CHANGE: The API is now async/await based
There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.
The properties of DAGNode and DAGLink are now in sync with the paths
that are used for resolving. This means that e.g. `name` is now called
`Name` and `size` is `Tsize`.
All return values from `resolve()` now conform to the [IPLD Data Model],
this means that e.g. links are no longer represented as
`{'/': "baseecodedcid"}`, but as [CID] instances instead.
For the full new API please see the [IPLD Formats spec].
[IPLD Data Model]: https://github.com/ipld/specs/blob/master/IPLD-Data-Model-v1.md
[CID]: https://github.com/multiformats/js-cid/
[IPLD Formats spec]: https://github.com/ipld/interface-ipld-format
BREAKING CHANGE: The API is now async/await based
There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.
The properties of DAGNode and DAGLink are now in sync with the paths
that are used for resolving. This means that e.g.
nameis now calledNameandsizeisTsize.All return values from
resolve()now conform to the IPLD Data Model,this means that e.g. links are no longer represented as
{'/': "baseecodedcid"}, but as CID instances instead.For the full new API please see the IPLD Formats spec.