feat: new IPLD Format API #124
Conversation
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
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #124 +/- ##
=======================================
+ Coverage 91.14% 93.14% +2%
=======================================
Files 14 16 +2
Lines 271 248 -23
=======================================
- Hits 247 231 -16
+ Misses 24 17 -7
Continue to review full report at Codecov.
|
| function deserialize (buffer, callback) { | ||
| const pbn = proto.PBNode.decode(buffer) | ||
| const deserialize = async (buffer) => { | ||
| const pbn = proto.PBNode.decode(Buffer.from(buffer)) |
There was a problem hiding this comment.
This will cause a new buffer to be created and the existing data to be copied into it. Unnecessary buffer copying has previously been identified as a performance bottleneck in our codebase.
There was a problem hiding this comment.
Previously we didn't copy this buffer before decoding it - did that cause a problem that this copy solves?
There was a problem hiding this comment.
Yes there was an issue, without this the tests fail. Should I just plug in the other Protocol Buffers library that works directly on Uint8Arrays (I've the code ready somewhere).
There was a problem hiding this comment.
protons is one of ours, can we not just fix it there instead of swapping out the whole library?
There was a problem hiding this comment.
That's the reason, if we use https://github.com/mapbox/pbf we get a faster thing and don't have to maintain it!
There was a problem hiding this comment.
I would descope swapping out deps for another PR, just make this about the API changes.
There was a problem hiding this comment.
Looking into the test failures, it seems it's related to returning Uint8Arrays instead of Buffers which is caused by this line in this PR - which is also a copy operation and so bad from a performance point of view.
The failures mostly seem to stem from other modules test to see if the thing they've been passed is a Buffer (instead of a Uint8Array or array-like object),
Not sure this is related to changing the API to be async/await - maybe you should try tackling that in a different PR to reduce the scope of this one?
There was a problem hiding this comment.
The problem is that bubbling up the changes up to js-ipfs is a huge pain, it takes weeks. Hence I'd like to do as little breaking releases as possible. Though I'll think about that as keeping it would result in less per regressions and I could make a clean PR which is exchanging the PB lib and changes the return value to Uint8Array.
There was a problem hiding this comment.
I could make a clean PR which is exchanging the PB lib and changes the return value to Uint8Array
I think this would be preferable
Bubbling up the changes up to js-ipfs is a huge pain, it takes weeks
Agreed. I think we can coordinate better as a team - maybe have a week where will all try to ship the same feature - be that base32 CIDs or Uint8Arrays, etc. Let's bring it up at next week's js-core catchup.
| name: this.name, | ||
| size: this.size, | ||
| cid: this._cid.toBaseEncodedString() | ||
| name: this.Name, |
There was a problem hiding this comment.
Should these keys get the go-name treatment too? E.g.
this._json = Object.freeze({
Name: this.Name,
Tsize: this.Tsize,
// etcThere was a problem hiding this comment.
I thought about this, but I'm unsure. What do you think?
There was a problem hiding this comment.
I would, for consistency - we can remove some of the mapping steps from the HTTP API endpoints then too.
|
Superseded by #127. |
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.