Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

WIP: new ipld format api #105

Closed
wants to merge 7 commits into from
Closed

WIP: new ipld format api #105

wants to merge 7 commits into from

Conversation

vmx
Copy link
Member

@vmx vmx commented Dec 6, 2018

This is an implementation of the new IPLD Format API. It hopefully shows that although the public API changes a lot, the changes make sense and it won't be too hard to upgrade.

The resolver is completely gone, all enumerable properties are the public API to resolve through now. See the resolver.spec.js test for how things work now.

BREAKING CHANGE: The IPLD Formats API is all new

See https://github.com/ipld/interface-ipld-format/
for the current specification
All tests except for named links pass.
Named links are now also available through a properties directly
on the DAGNode object. This makes IPFS-style traversals possible.
Only the newly proposed IPLD Format API is left.
There were dependencies which are no longer needed or are
devDependencies now.
@ghost ghost assigned vmx Dec 6, 2018
@ghost ghost added the status/in-progress In progress label Dec 6, 2018
@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #105 into master will decrease coverage by 0.59%.
The diff coverage is 95.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #105     +/-   ##
=========================================
- Coverage   91.35%   90.75%   -0.6%     
=========================================
  Files          13       13             
  Lines         266      238     -28     
=========================================
- Hits          243      216     -27     
+ Misses         23       22      -1
Impacted Files Coverage Δ
src/dag-link/index.js 88.88% <100%> (-1.12%) ⬇️
src/dag-node/addNamedLink.js 100% <100%> (ø)
src/dag.proto.js 100% <100%> (ø) ⬆️
src/dag-node/addLink.js 100% <100%> (+9.09%) ⬆️
src/dag-node/util.js 70% <100%> (-21.31%) ⬇️
src/dag-node/create.js 100% <100%> (+12.5%) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/dag-node/index.js 85.71% <83.87%> (-2.29%) ⬇️
src/dag-node/rmLink.js 92.3% <88.88%> (-0.55%) ⬇️
src/ipld-format.js 98% <98%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56ed9a...985c7f4. Read the comment docs.

@achingbrain
Copy link
Member

If we're reviewing the API, let's remove the size attribute from DAGLinks.

It's unreliable, expensive to calculate and leaks serialization details into the application layer.

@achingbrain
Copy link
Member

achingbrain commented Dec 21, 2018

What's the thought behind using pbf over the current protons module in this PR?

I'm looking at performance optimisations and our protobuf serialization is a very hot code path so will be looking at that soon. I don't want to spend time optimising the wrong module!

@vmx
Copy link
Member Author

vmx commented Dec 21, 2018

What's the thought behind using pbf over the current protons module in this PR?

The main reason was that I didn't want a dependency on Node.js Buffers but be able to use ArrayBuffers. I also prefer not to maintain forks of something. In addition to that the mapbox folks (who are maintaining pbf) really know what they are doing. And finally I preferred having the schema compiled once and committed rather then doing it on load/runtime.

@achingbrain
Copy link
Member

achingbrain commented Jan 3, 2019

If the protons -> pbf change is worth doing it could be pulled out of this PR and done separately, right? No need to break the public API for an internal serialisation library switch.

Obviously if this module starts emitting ArrayBuffers instead of Node Buffers it's going to upset things, but we can Buffer.from before outputting things. According to the docs it'll create a view on the ArrayBuffer instead of copying it.

@vmx
Copy link
Member Author

vmx commented May 8, 2019

Superseded by #127.

@vmx vmx closed this May 8, 2019
@ghost ghost removed the status/in-progress In progress label May 8, 2019
@vmx vmx deleted the new-ipld-format-api branch May 8, 2019 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants