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

Backwards compatible pure data model API #184

Merged
merged 8 commits into from
Jul 9, 2020
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jun 5, 2020

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.

@Gozala Gozala force-pushed the pure-data-model branch from ceed97e to 2eaff18 Compare June 5, 2020 22:20
@vmx
Copy link
Member

vmx commented Jun 8, 2020

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.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, just a few small points.

test/dag-node-test.js Outdated Show resolved Hide resolved
test/dag-node-test.js Show resolved Hide resolved
src/serialize.js Outdated Show resolved Hide resolved
src/dag-node/sortLinks.js Outdated Show resolved Hide resolved
@Gozala Gozala requested a review from achingbrain June 9, 2020 00:42
@Gozala
Copy link
Contributor Author

Gozala commented Jun 9, 2020

Summary of few updates I've made:

  1. Pull chai from aegir.
  2. Remove Uint8Array to Buffer casting in favor of doing it in protons (See feat: support Uint8Array input in place of Buffer ipfs/protons#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.
  1. Replaced immutable sortLinks with mutable version. Immutable version was not used anywhere else in this package.

package.json Outdated Show resolved Hide resolved
src/dag-node/rmLink.js Outdated Show resolved Hide resolved
src/dag-node/dagNode.js Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me (once the protons dep is updated, and I wouldn't mind seeing consistency retained with arrow function parens as per notes I've left). I hope we have solid integration tests covering all of this when it propagates upward!

@Gozala
Copy link
Contributor Author

Gozala commented Jun 24, 2020

@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

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 very nice work

this will probably have to wait for @vmx to return from vacation to get published, unless there's some urgency

@Gozala Gozala changed the title WIP: backwards compatible pure data model API Backwards compatible pure data model API Jun 25, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Jul 8, 2020

@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 vmx merged commit c28ca60 into ipld:master Jul 9, 2020
@vmx
Copy link
Member

vmx commented Jul 9, 2020

@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
Copy link
Contributor Author

Gozala commented Jul 9, 2020

@vmx that sounds good to me. Thanks

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.

4 participants