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

feat: add types #189

Merged
merged 10 commits into from
Feb 18, 2021
7 changes: 6 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
language: node_js
cache: npm
branches:
only:
- master
- /^release\/.*$/
Comment on lines +3 to +6
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that it doesn't run CI if a branch is pushed without doing a PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's not enabled, you can configure this setting in travis to build every non-pr branch

stages:
- check
- test
- cov

node_js:
- '10'
- '14'
- '15'

os:
- linux
Expand Down
18 changes: 11 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
"leadMaintainer": "Volker Mische <volker.mische@gmail.com>",
"main": "src/index.js",
"scripts": {
"prepare": "run-s prepare:*",
"prepare:proto": "pbjs -t static-module -w commonjs --force-number --no-verify --no-delimited --no-create --no-beautify --no-defaults --lint eslint-disable -o src/dag.js ./src/dag.proto",
"prepare:proto:types": "pbts -o src/dag.d.ts src/dag.js",
"prepare:types": "aegir build --no-bundle",
"prepare:copy": "cp ./src/*.d.ts ./dist/src",
"test": "aegir test",
"test:browser": "aegir test --target browser",
"test:node": "aegir test --target node",
Expand All @@ -13,7 +18,6 @@
"release": "aegir release",
"release-minor": "aegir release --type minor",
"release-major": "aegir release --type major",
"build": "aegir build",
Copy link
Member

Choose a reason for hiding this comment

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

I guess the removal of the build command is intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - prepare is run after installing and before publishing so there's no need for a separate build command.

"coverage": "aegir coverage",
"coverage-publish": "aegir coverage publish"
},
Expand Down Expand Up @@ -66,18 +70,18 @@
},
"dependencies": {
"cids": "^1.0.0",
"class-is": "^1.1.0",
"multicodec": "^2.0.0",
"multihashing-async": "^2.0.0",
"protons": "^2.0.0",
"protobufjs": "^6.10.2",
vmx marked this conversation as resolved.
Show resolved Hide resolved
"reset": "^0.1.0",
"run": "^1.4.0",
"stable": "^0.1.8",
"uint8arrays": "^1.0.0"
"uint8arrays": "^2.0.5"
},
"devDependencies": {
"aegir": "^25.0.0",
"aegir": "^30.3.0",
"multibase": "^3.0.0",
"multihashes": "^3.0.0"
}
"npm-run-all": "^4.1.5"
},
"types": "dist/src/index.d.ts"
}
22 changes: 13 additions & 9 deletions src/dag-link/dagLink.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
'use strict'

const CID = require('cids')
const withIs = require('class-is')
const uint8ArrayFromString = require('uint8arrays/from-string')

// Link represents an IPFS Merkle DAG Link between Nodes.
/**
* Link represents an IPFS Merkle DAG Link between Nodes.
*/
class DAGLink {
/**
* @param {string | undefined | null} name
* @param {number} size
* @param {CID | string | Uint8Array} cid
*/
constructor (name, size, cid) {
if (!cid) {
throw new Error('A link requires a cid to point to')
Expand All @@ -14,13 +20,11 @@ class DAGLink {
// assert(size, 'A link requires a size')
// note - links should include size, but this assert is disabled
// for now to maintain consistency with go-ipfs pinset
this.Name = name || ''
this.Tsize = size
this.Hash = new CID(cid)
Comment on lines +23 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Having the values read-only is something dag-pb did (IIRC) from the very beginning. I'm not sure if it is important or not (if it's not important for unixfs, it is probably not).

@rvagg do you have oponions on that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not something that unixfs uses.

Copy link
Member

Choose a reason for hiding this comment

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

Please change it back to the original code. I don't see a reason to make a change that might break things in a subtle way if we don't have to.


Object.defineProperties(this, {
Name: { value: name || '', writable: false, enumerable: true },
Tsize: { value: size, writable: false, enumerable: true },
Hash: { value: new CID(cid), writable: false, enumerable: true },
_nameBuf: { value: null, writable: true, enumerable: false }
})
this._nameBuf = null
vmx marked this conversation as resolved.
Show resolved Hide resolved
}

toString () {
Expand Down Expand Up @@ -52,4 +56,4 @@ class DAGLink {
}
}

exports = module.exports = withIs(DAGLink, { className: 'DAGLink', symbolName: '@ipld/js-ipld-dag-pb/daglink' })
module.exports = DAGLink
1 change: 0 additions & 1 deletion src/dag-link/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
'use strict'

exports = module.exports = require('./dagLink')
exports.util = require('./util')
10 changes: 7 additions & 3 deletions src/dag-link/util.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
'use strict'

const DAGLink = require('./dagLink')
const DAGLink = require('./')

/**
* @param {*} link
*/
function createDagLinkFromB58EncodedHash (link) {
return new DAGLink(
link.Name || link.name || '',
Expand All @@ -10,5 +13,6 @@ function createDagLinkFromB58EncodedHash (link) {
)
}

exports = module.exports
exports.createDagLinkFromB58EncodedHash = createDagLinkFromB58EncodedHash
module.exports = {
createDagLinkFromB58EncodedHash
}
18 changes: 16 additions & 2 deletions src/dag-node/addLink.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
'use strict'

const sortLinks = require('./sortLinks')
const DAGLink = require('../dag-link')
const DAGLink = require('../dag-link/dagLink')

/**
* @typedef {import('./dagNode')} DAGNode
* @typedef {import('../types')} DAGLinkLike
*/

/**
* @param {*} link
* @returns {DAGLink}
*/
const asDAGLink = (link) => {
if (DAGLink.isDAGLink(link)) {
if (link instanceof DAGLink) {
// It's a DAGLink instance
// no need to do anything
return link
Expand All @@ -21,9 +30,14 @@ const asDAGLink = (link) => {
}

// It's a Object with name, multihash/hash/cid and size
// @ts-ignore
return new DAGLink(link.Name || link.name, link.Tsize || link.size, link.Hash || link.multihash || link.hash || link.cid)
}

/**
* @param {DAGNode} node
* @param {DAGLink | DAGLinkLike} link
*/
const addLink = (node, link) => {
const dagLink = asDAGLink(link)
node.Links.push(dagLink)
Expand Down
50 changes: 35 additions & 15 deletions src/dag-node/dagNode.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
'use strict'

const withIs = require('class-is')
const sortLinks = require('./sortLinks')
const DAGLink = require('../dag-link/dagLink')
const { serializeDAGNode } = require('../serialize.js')
const { createDagLinkFromB58EncodedHash } = require('../dag-link/util')
const { serializeDAGNode } = require('../serialize')
const toDAGLink = require('./toDagLink')
const addLink = require('./addLink')
const rmLink = require('./rmLink')
const uint8ArrayFromString = require('uint8arrays/from-string')
const uint8ArrayToString = require('uint8arrays/to-string')

/**
* @typedef {import('cids')} CID
* @typedef {import('../types').DAGLinkLike} DAGLinkLike
*/

class DAGNode {
/**
*@param {Uint8Array | string} [data]
* @param {(DAGLink | DAGLinkLike)[]} links
* @param {number | null} [serializedSize]
*/
constructor (data, links = [], serializedSize = null) {
if (!data) {
data = new Uint8Array(0)
Expand All @@ -27,19 +37,17 @@ class DAGNode {
throw new Error('Passed \'serializedSize\' must be a number!')
}

links = links.map((link) => {
return DAGLink.isDAGLink(link)
const sortedLinks = links.map((link) => {
return link instanceof DAGLink
? link
: DAGLink.util.createDagLinkFromB58EncodedHash(link)
: createDagLinkFromB58EncodedHash(link)
})
sortLinks(links)
sortLinks(sortedLinks)

Object.defineProperties(this, {
Data: { value: data, writable: false, enumerable: true },
Links: { value: links, writable: false, enumerable: true },
_serializedSize: { value: serializedSize, writable: true, enumerable: false },
_size: { value: null, writable: true, enumerable: false }
})
this.Data = data
this.Links = sortedLinks
this._serializedSize = serializedSize
this._size = null
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this change is the resolver. If you have a call like

resolver.resolve(blob, '_size')

It would return something like { value: null, remainderPath: '' } although it should throw an error as the property doesn't exist.

Were there other reasons expect for better readability for this change?

Copy link
Member

Choose a reason for hiding this comment

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

This is still an issue. I propose changing it back to the original code.

Copy link
Member Author

@achingbrain achingbrain Feb 18, 2021

Choose a reason for hiding this comment

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

If you use Object.defineProperties, the properties are only set at runtime so you lose type safety.

I've updated the PR so only the properties that should be non-enumerable are set using Object.defineProperties, the others are set in the normal way so you get the best of both worlds.

}

toJSON () {
Expand All @@ -63,23 +71,35 @@ class DAGNode {
this._size = null
}

/**
* @param {DAGLink | import('../types').DAGLinkLike} link
*/
addLink (link) {
this._invalidateCached()
return addLink(this, link)
}

/**
* @param {DAGLink | string | CID} link
*/
rmLink (link) {
this._invalidateCached()
return rmLink(this, link)
}

// @returns {Promise.<DAGLink>}
/**
* @param {import('./toDagLink').ToDagLinkOptions} [options]
*/
toDAGLink (options) {
return toDAGLink(this, options)
}

serialize () {
return serializeDAGNode(this)
const buf = serializeDAGNode(this)

this._serializedSize = buf.length
Copy link
Member

Choose a reason for hiding this comment

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

A test for that fix would be great.


return buf
}

get size () {
Expand All @@ -98,4 +118,4 @@ class DAGNode {
}
}

exports = module.exports = withIs(DAGNode, { className: 'DAGNode', symbolName: '@ipld/js-ipld-dag-pb/dagnode' })
module.exports = DAGNode
17 changes: 14 additions & 3 deletions src/dag-node/rmLink.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,25 @@
const CID = require('cids')
const uint8ArrayEquals = require('uint8arrays/equals')

/**
* @typedef {import('../dag-link/dagLink')} DAGLink
*/

/**
*
* @param {import('./dagNode')} dagNode
* @param {string | CID | Uint8Array | DAGLink} nameOrCid
*/
const rmLink = (dagNode, nameOrCid) => {
let predicate = null

// It's a name
if (typeof nameOrCid === 'string') {
predicate = (link) => link.Name === nameOrCid
} else if (nameOrCid instanceof Uint8Array || CID.isCID(nameOrCid)) {
predicate = (link) => uint8ArrayEquals(link.Hash, nameOrCid)
predicate = (/** @type {DAGLink} */ link) => link.Name === nameOrCid
} else if (nameOrCid instanceof Uint8Array) {
predicate = (/** @type {DAGLink} */ link) => uint8ArrayEquals(link.Hash.bytes, nameOrCid)
} else if (CID.isCID(nameOrCid)) {
Copy link
Member

Choose a reason for hiding this comment

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

A test for this fix would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH DAGNode.rmLink accepts DAGLink | string | CID so the type checker would object to you passing a Uint8Array in, but not everyone uses a type checker.

predicate = (/** @type {DAGLink} */ link) => uint8ArrayEquals(link.Hash.bytes, nameOrCid.bytes)
}

if (predicate) {
Expand Down
12 changes: 11 additions & 1 deletion src/dag-node/sortLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@
const sort = require('stable')
const uint8ArrayCompare = require('uint8arrays/compare')

/**
* @typedef {import('../dag-link/dagLink')} DAGLink
*/

/**
*
* @param {DAGLink} a
* @param {DAGLink} b
*/
const linkSort = (a, b) => {
const buf1 = a.nameAsBuffer
const buf2 = b.nameAsBuffer
Expand All @@ -12,7 +21,8 @@ const linkSort = (a, b) => {

/**
* Sorts links in place (mutating given array)
* @param {Array} links
*
* @param {DAGLink[]} links
* @returns {void}
*/
const sortLinks = (links) => {
Expand Down
17 changes: 14 additions & 3 deletions src/dag-node/toDagLink.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
'use strict'

const DAGLink = require('../dag-link/dagLink')
const DAGLink = require('../dag-link')
const genCid = require('../genCid')

/*
/**
* toDAGLink converts a DAGNode to a DAGLink
*
* @typedef {import('../genCid').GenCIDOptions} GenCIDOptions
*
* @typedef {object} ToDagLinkExtraOptions
* @property {string} [name]
*
* @typedef {GenCIDOptions & ToDagLinkExtraOptions} ToDagLinkOptions
*
* @param {import('./dagNode')} node
* @param {ToDagLinkOptions} options
*/
const toDAGLink = async (node, options = {}) => {
const nodeCid = await genCid.cid(node.serialize(), options)
const buf = node.serialize()
const nodeCid = await genCid.cid(buf, options)
return new DAGLink(options.name || '', node.size, nodeCid)
}

Expand Down
Loading