feat: update DAGLink and DAGNode to have an immutable API #6

daviddias · 2016-11-14T10:45:00Z

WIP

@dignifiedquire @victorbjelkholm @haadcode did a push on this so that we can get this final API stable, while the API for IPLD Formats are still hot, I'm afraid that if we give it a lot more time, people will start using all of that async ugliness because of the lack of immutability here and then it becomes harder to change.

I'm finishing this up, but API is there, would love your review ASAP

Agree with the API
Update remaining tests
Update README

note: This PR also cuts the umbilical chord to the merkleDAG version in Go, which created a funky api in JS

dignifiedquire

I think it would be good to keep a toJSON method, to make it more consistent rather than a property .json. For both node and link

dignifiedquire · 2016-11-14T11:07:59Z

src/dag-node.js

+      get () {
+        return data
+      },
+      set: immutableError


You can make this nicer as you are already using class

class Hello { get myImmutableProp () { return this._secretProp } set myImmutableProp () { throw new Error('fail') } }

well, that is what I did, but defined the function above so that I could reuse it

right, nevermind, I got what you meant now :) and agreed 👍

dignifiedquire · 2016-11-14T11:09:48Z

src/dag-link.js

-      Name: this.name,
-      Size: this.size,
-      Hash: mh.toB58String(this.hash)
+    this.json = {


This should be defined as immutable as the props in DagNode

agreed 👍

daviddias · 2016-11-14T11:26:09Z

I think it would be good to keep a toJSON method, to make it more consistent rather than a property .json. For both node and link

I tend to agree, will update

haadcode · 2016-11-14T11:34:36Z

I have quite a few comments for this, but can't review it atm. Will get back to this asap.

daviddias · 2016-11-16T10:54:15Z

src/dag-node.js

+            // 3. create new node
+          }
+        }
+        callback(new Error('invalid arguments'))


…lder, make tests pass again

… write more tests

daviddias · 2016-11-20T18:26:15Z

All right, this is ready to be reviewed so that I can go forward and bubble up updates across the other repos.

Big wins from this PR:

Got a tons of feedback from @haadcode
The whole dagPB became more functional
Code structure improved, smaller files with clear goals
Better test coverage
Documentation was also improved
Code overall improved a ton

@haadcode @dignifiedquire @victorbjelkholm would like a final 👍 so that I can move forward.

Thank you!

haadcode · 2016-11-21T07:34:38Z

src/index.js

-exports.DAGLink = require('./dag-link.js')
-exports.resolver = require('./resolver.js')
-exports.util = require('./util.js')
+exports.DAGNode = require('./dag-node')


This will make const d = new DAGNode() possible for users. I thought we didn't want that to be exposed directly so that users are forced to user .create and have no confusion as to how to create DAGNodes.

haadcode · 2016-11-21T07:35:19Z

src/index.js

-exports.resolver = require('./resolver.js')
-exports.util = require('./util.js')
+exports.DAGNode = require('./dag-node')
+exports.DAGLink = require('./dag-link')



Same as above, this will expose DAGLink to the consumers.

haadcode · 2016-11-21T07:40:01Z

src/util.js

-  const dagNode = new DAGNode(buf, links)
-
-  callback(null, dagNode)
+  DAGNode.create(buf, links, callback)


@diasdavid we talked about the IPLD format API and this is pretty much what I meant how it forces sub-optimal code: deserialize depends on DAGNode and DAGNode depends on serialize (serialize and deserialize being both in the same file) essentially creating a cyclic dependency between the two. For the sake of getting this shipped, let's not focus on that but keep this in mind for future discussions.

We could break util into separate files for each function, avoiding that 'cyclic' dependency, do note that in CJS, modules loaded are cached, solving this potential issue without any intervention.

Nevertheless, this is not a IPLD Format issue, if an issue at all, is just because we want the DAGNode instances to be immutable and easy to work with and so, we need the serialize version upon its creation.

haadcode · 2016-11-21T07:40:33Z

Couple of small comments, otherwise LGTM!

dignifiedquire · 2016-11-23T08:48:50Z

README.md

+Create a DAGNode.
+
+```JavaScript
+DAGNode.create("data", links, (err, dagNode) => {


dignifiedquire · 2016-11-23T08:49:07Z

README.md

+#### Create a DAGNode
+
+```JavaScript
+// TODO


TODO: please fill or remove before merge

dignifiedquire · 2016-11-23T08:49:16Z

README.md

+#### Add and remove a Link
+
+```JavaScript
+// TODO


TODO: please fill or remove before merge

dignifiedquire · 2016-11-23T08:49:52Z

README.md

+const DAGNode = dagPB.DAGNode
+```
+
+#### DAGNode.create(data, links, hashAlg, callback)


Please document the argument types

dignifiedquire · 2016-11-23T08:50:11Z

README.md

+}
+```
+
+#### addLink(node, link, callback)


Please document the argument types

dignifiedquire · 2016-11-23T08:56:05Z

src/dag-node/create.js

+    }
+
+    return new DAGLink(l.name || l.Name,
+                       l.size || l.Size,


indentation is off

dignifiedquire · 2016-11-23T09:07:31Z

src/dag-node/util.js

+  }
+
+  return data
+}


this can be simplified to

function cloneData (dagNode) { assert(Buffer.isBuffer(dagNode.data), 'Not a valid dagnode') const data = new Buffer(dagNode.data.length) dagNode.data.copy(data) return data }

dignifiedquire · 2016-11-23T09:09:40Z

src/dag-node/util.js

+}
+
+function cloneLinks (dagNode) {
+  return dagNode.links.length > 0 ? dagNode.links.slice() : []


no need for the length check, [].slice() === []

dignifiedquire · 2016-11-23T09:10:33Z

src/dag-node/util.js

+}
+
+function linkSort (a, b) {
+  return (new Buffer(a.name || '', 'ascii').compare(new Buffer(b.name || '', 'ascii')))


This is super wasteful, the link names should be converted to buffers once and then sorted, not on every sorting call.

dignifiedquire · 2016-11-23T09:12:28Z

src/util.js

 }

+exports.serialize = serialize
+exports.deserialize = deserialize
+exports.cid = cid


👎 on the whole util interface, it seems arbitrary and not very useful. If these methods are needed why are they not exposed at the top level?

@dignifiedquire go through https://github.com/ipld/interface-ipld-format and ipld/js-ipld#60 (comment)

tl;dr; it is what the IPLD Resolver needs and what was defined on interface-ipld-format

feat: update DAGLink and DAGNode to have an immutable API

4bdb48b

daviddias added the Needs Review label Nov 14, 2016

daviddias self-assigned this Nov 14, 2016

dignifiedquire reviewed Nov 14, 2016

View reviewed changes

haadcode mentioned this pull request Nov 14, 2016

Refactor feat/immutable PR #7

Merged

daviddias commented Nov 16, 2016

View reviewed changes

src/dag-node.js

// 3. create new node

}

}

callback(new Error('invalid arguments'))

Copy link

Member Author

daviddias Nov 16, 2016

Choose a reason for hiding this comment

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

haadcode and others added 8 commits November 20, 2016 13:44

feat(refactor): new API proposal

b56d797

feat: refactor, structure code, make DAGNode funcs inside the same fo…

ea904a7

…lder, make tests pass again

feat: add a same create pattern api for DAGLink

6a3531d

fix: that linting

301d0b0

docs: revamp README documentation to the new API

6e3ab23

fix: DAGLink tests

eec00da

feat+fix: complete addLink and rmLink functions + fix their tests and…

710e53f

… write more tests

feat: IPLD Resolver updated, all tests passing

f53e0c8

daviddias force-pushed the feat/immutable branch from 8f7aadc to f53e0c8 Compare November 20, 2016 18:23

haadcode reviewed Nov 21, 2016

View reviewed changes

daviddias mentioned this pull request Nov 22, 2016

Awesome dagPB.DAGNode API ipfs/js-ipfs#617

Closed

dignifiedquire suggested changes Nov 23, 2016

View reviewed changes

daviddias added 2 commits November 23, 2016 17:14

docs: add examples to readme

0ea67c9

docs: document types of functions in the readme

a6b9350

daviddias removed the Needs Review label Nov 23, 2016

fix: apply code review

2b1a356

daviddias force-pushed the feat/immutable branch from b3b9385 to 2b1a356 Compare November 23, 2016 17:39

fix: serialization of empty node

ae71f26

daviddias merged commit 44c86a4 into master Nov 24, 2016

daviddias deleted the feat/immutable branch November 24, 2016 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update DAGLink and DAGNode to have an immutable API #6

feat: update DAGLink and DAGNode to have an immutable API #6

daviddias commented Nov 14, 2016 •

edited

Loading

dignifiedquire left a comment

dignifiedquire Nov 14, 2016

daviddias Nov 14, 2016

daviddias Nov 14, 2016 •

edited

Loading

dignifiedquire Nov 14, 2016

daviddias Nov 14, 2016

daviddias commented Nov 14, 2016

haadcode commented Nov 14, 2016

daviddias Nov 16, 2016

daviddias commented Nov 20, 2016

haadcode Nov 21, 2016

haadcode Nov 21, 2016

haadcode Nov 21, 2016

daviddias Nov 21, 2016 •

edited

Loading

haadcode commented Nov 21, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

dignifiedquire Nov 23, 2016

daviddias Nov 23, 2016 •

edited

Loading

dignifiedquire Nov 23, 2016

feat: update DAGLink and DAGNode to have an immutable API #6

feat: update DAGLink and DAGNode to have an immutable API #6

Conversation

daviddias commented Nov 14, 2016 • edited Loading

dignifiedquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Nov 14, 2016

haadcode commented Nov 14, 2016

Choose a reason for hiding this comment

daviddias commented Nov 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

haadcode commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Nov 14, 2016 •

edited

Loading

daviddias Nov 14, 2016 •

edited

Loading

daviddias Nov 21, 2016 •

edited

Loading

daviddias Nov 23, 2016 •

edited

Loading