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

Remove DAGNode.create? #132

Closed
achingbrain opened this issue May 14, 2019 · 2 comments
Closed

Remove DAGNode.create? #132

achingbrain opened this issue May 14, 2019 · 2 comments

Comments

@achingbrain
Copy link
Member

Sort of related to #131 but if DAGNode.create can now calculate the serialized size of a node synchronously, why not do that in the DAGNode constructor if the serialized size is not passed and remove the .create function?

@vmx
Copy link
Member

vmx commented May 16, 2019

I had a look. the problem is that we would need to call serialize() which does a call to DAGNode.isDAGNode(). So you'd have a circular reference that would make things fail.

@achingbrain
Copy link
Member Author

It only does a call to DAGNode.isDAGNode() to find out if the data argument passed to util.serialize is a DAGNode, which it should not be if we are in the constructor of DAGNode outside of programmer error so that could be factored out and you'd have an internal _serialize function that did the same thing as util.serialize just without that check which util.serialize would call.

...

Anyway I've found this is only a concern when creating trees of DAGNodes and DAGLinks prior to serialization so we don't even know the serialized size at that point.

We only assert on serialized size in the DAGNode constructor if it's non-zero in order to do this sort of thing:

const node = new DAGNode(buf, links, 0) // dunno

How about defaulting it (and the links?) in the constructor?

class DAGNode {
  constructor (data, links = [], serializedSize = 0) {
    if (serializedSize !== 0) {
      assert(serializedSize, 'A DAGNode requires it\'s serialized size')
    }
...

This way you can use new DAGNode(...) and new DAGLink(...) symmetrically, ditch the DAGNode.create method and you don't need the weird 0 argument at the end of the DAGNode constructor.

If fact you could probably remove the assert from the constructor entirely as it seems pretty low value at this point.

vmx added a commit that referenced this issue May 22, 2019
BREAKING CHANGE: DAGNode.create() is removed

Instead of `DAGNode.create()`, please use `new DAGNode()` instead. It
takes the same parameters and is compatible to `create()`.

Example:

Prior to this change:

    const node = DAGNode.create('some data', links)

Now:

    const node = new DAGNode('some data', links)

Closes #132.
vmx added a commit that referenced this issue Jul 17, 2019
BREAKING CHANGE: DAGNode.create() is removed

Instead of `DAGNode.create()`, please use `new DAGNode()` instead. It
takes the same parameters and is compatible to `create()`.

Example:

Prior to this change:

    const node = DAGNode.create('some data', links)

Now:

    const node = new DAGNode('some data', links)

Closes #132.
@vmx vmx closed this as completed in 029174d Jul 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants