Make dag-pb an immutable 'class'

[daviddias]

It is very cumbersome to use DAGNode class in the way it was designed originally, specially now with the async crypto constraints that require a callback to mutate state mutation on each instance (literally, bleh).

I would like to make the strong case to make the methods of this DAGNode class actually function present in the dagPB.util, so that any mutation returns an instance of a new instance.

Benefits:

• no more node.multihash((err, multihash) => {})
• no more node.toJSON((err, nodeJSON) => {})
• more of node.multihash (calculate once, use it every time)
• more of node.json (calculate once, use it every time)

//cc @haadcode @dignifiedquire @victorbjelkholm

[dignifiedquire]

sounds good to me

[haadcode]

I'm also in favor of making it more functional. However, I would propose the following namespacing:

DAGNode.toJSON(node).then((json) => ...)
DAGNode.getMultihash(node).then((hash) => ...)
// and same with callbacks

This makes it easier to reason about the functionality as well as to find the tools to do the conversion, requiring to only include one module for the conversions and reduces the cognitive overload (ie. doesn't require me to understand what PB means).

Haven't checked what the current DAGNode code is (whether it's a class or a function), but if it's a class, these functions can be part of the class by making them static methods of the class, ie.

class DAGNode {
  ...
  static toJSON(node) { ... }
  static getMultihash(node) { ... }
}

Thoughts?

[daviddias]

@haadcode it is better if you could check the DAGNode code first, my proposal on making it 'functional' was specifically to move from methods to functions that don't alter internal state. What you have proposed is essentially what we have, but just with a promise wrapper.

If we move forward with the initial proposal, it would mean that an instance of the DAGNode class would have the properties .json and .multihash, instead of having a method.

[haadcode]

@diasdavid let me clear some confusion here. static methods are different to instance methods, meaning they're not part of an instance of the class (== object) and they don't contain state. A static method is basically the same as a function, just different terminology (OO vs. functional I suppose).

Eg. now you can do node.toJSON() but if the .toJSON is a static method, node.toJSON() would scream undefined.

So how it would work, as a longer example, is:

const node = new DAGNode(...) // instance of the dag node
DAGNode.toJSON(node).then((json) => {
  console.log(json) // { "Data": ..., "Links": ... }
})

of with a callback:

const node = new DAGNode(...) // instance of the dag node
DAGNode.toJSON(node, (err, json) => {
  console.log(json) // { "Data": ..., "Links": ... }
})

In that example, node doesn't change its state. This is the important difference. Take a look at https://github.com/haadcode/ipfs-log/blob/master/src/entry.js as an example. The Entry class doesn't contain any state, just static methods that always return data. You can't instantiate an instance of Entry at all, but have to create an Entry object by calling Entry.create(...), that means entry = new Entry(...) is not possible.

So if we look at https://github.com/ipld/js-ipld-dag-pb/blob/master/src/dag-node.js#L177, I think all that is needed to make it work "more functional", is to make that method static and add the node as the first argument: static toJSON (node, callback) { ... (ie. node is the DAGNode instance from which we want to get the JSON from). The method there already returns data as opposed to changing the state of the DAGNode (afaict). The same approach can be used in .multihash() (which I guess changes the state atm).

Does that make sense? I believe this is the cleanest and quickest way forward. Let me know if I can help with this.

As a side note, I would really like to wrap the methods to a Promise so that we're ready for await and in the future the above example can be written as:

const node = new DAGNode(...)
const json = await DAGNode.toJSON(node)

[dignifiedquire]

What is the benefit of static methods over something like this:

const node = new DAGNode(..)
node.toJSON((err, json) => {
  // the state of node has not been modified, similar to how instance methods on immutable.js work
})

// adding a link (sync for easier example, could work async the same way)
const node2 = node.addLink(mylink)
// node2 is a new instance, and node was not modified at all

This api would be much more ergonomic and nicer to work with, then having a bunch of static methods to call. They could be easily implemented using those though:

toJSON(cb) {
  DAGNode.toJSON(this, cb)
}

[haadcode]

@dignifiedquire generally I agree that having data conversion methods as part of the instance are a nicer solution (in OO terms), but given the async nature of JS and the fact that they have already become async, I think what @diasdavid is proposing here makes more sense. It keeps API simpler to reason about imo.

If .toJSON was sync, then I would definitely like to have it as part of the instance. Actually, imho, the nicest way to do such a data conversion would be node.asJson (no function call, not a property, but a "give me this data structure as this other data structure").

As for the second example, that really mixes the two worlds (OO and functional) and makes it hard to reason how to use the API. So in that case, a better way would be const node2 = addLink(node, myLink) (functional) or node.addLink(myLink) // node's state changed (OO).

[haadcode]

Removing this comment, realized the async code comes from the new Async Crypto PR

[daviddias]

It's done wooooo! 🎉🎉🎉🎉🎉

[daviddias]

Follow the merging here: ipfs/js-ipfs#617