Backwards compatible pure data model API

ipld / js-ipld-dag-pb

JavaScript Implementation of the MerkleDAG Nodes with Protobuf.

MIT License

69 stars 20 forks source link

Backwards compatible pure data model API #184

Closed Gozala closed 4 years ago

Gozala commented 4 years ago

This pull request attempts to add pure Data Model API interop (as per #173) in a backwards compatible way. High level overview of the changes are:

Getters / Setters are replaced by non-writable public properties.
Some internal ops that add / remove links do mutate arrays in place instead of swapping arrays (due to introduce non-writable constraint).
serialization code now handles ArrayBuffer and TypedArray for node.Data.

Only visible change is that node.Links no longer returns links mapped to pure data, but rather DAGLink instances. However it should not matter in practice because those were made to behave just like pure data would. Although tests had to be change to use .containSubset instead of .eql because later fails when prototype chain is different.

vmx commented 4 years ago

Wow, nice work. I just tested it also with js-ipld and it just worked. If @achingbrain is happy with those changes, I don't see a reason for not making those changes.

Gozala commented 4 years ago

Summary of few updates I've made:

Pull chai from aegir.
Remove Uint8Array to Buffer casting in favor of doing it in protons (See https://github.com/ipfs/protons/pull/13)
- Current patch changes protons dep to point to my fork. Once it lands and published I'll update this to point to new verison.
- ArrayBuffer as Data is no longer supported, it's just Uint8Array now. Mostly to keep things simple on protons side.
Replaced immutable sortLinks with mutable version. Immutable version was not used anywhere else in this package.

Gozala commented 4 years ago

@achingbrain I believe I've addressed all your comments and comments from @rvagg. @vmx I hope you agree with my arguments to decouple this from switch to https://github.com/mapbox/pbf, I do not know what errors may arise from that change & I do not want to block my primary work on that.

Please let me know if there is anything else to be done, or if this can land.

Thanks

Gozala commented 4 years ago

@vmx any chance you could get to this sometime this week ? I can't make any more progress on ipfs/js-ipfs#3081 without this.

vmx commented 4 years ago

@Gozala any objections if I release this as 0.19? It's been quite a big change and I don't want to have regressions sneaked it.

Gozala commented 4 years ago

@vmx that sounds good to me. Thanks