Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No more fs.readFileSync and different hash functions #50

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,35 @@

## Table of Contents

- [Install](#install)
- [Architecture](#architecture)
- [Usage](#usage)
- [API](#api)
- [Contribute](#contribute)
- [License](#license)
* [Table of Contents](#table-of-contents)
* [Install](#install)
* [Architecture](#architecture)
* [Usage](#usage)
* [API](#api)
+ [DAGNode Class](#dagnode-class)
- [`addNodeLink`](#addnodelink)
- [`addRawLink`](#addrawlink)
- [`updateNodeLink`](#updatenodelink)
- [`removeNodeLink`](#removenodelink)
- [`removeNodeLinkByHash`](#removenodelinkbyhash)
- [`copy`](#copy)
- [`size`](#size)
- [`links`](#links)
- [`multihash(fn)`](#multihashfn)
- [`marshal`](#marshal)
- [`unMarshal`](#unmarshal)
- [`getPBNode`](#getpbnode)
- [`makeLink`](#makelink)
+ [DAGLink Class](#daglink-class)
+ [DAGService](#dagservice)
- [`put`](#put)
- [`putStream`](#putstream)
- [`get`](#get)
- [`getStream`](#getstream)
- [`getRecursive`](#getrecursive)
- [`getRecursiveStream`](#getrecursivestream)
- [`remove`](#remove)
* [License](#license)
Copy link
Member

Choose a reason for hiding this comment

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

sweeet! Thank you 👍

📜 for the 📜 god


## Install

Expand Down Expand Up @@ -99,10 +122,12 @@ var node = new ipfsMDAG.DAGNode([<data>, <[links]>])

> (property) an array of `DAGLink`s belonging to the node

#### `multihash`
#### `multihash(fn)`
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't expect an actual function, just the hash identifier. Can we use a better name for this arg?

Also, if there is a default. It should be [<fn>]


> returns the multihash (default: sha2-256)

`fn` can be any hash function that [`multihashing`](https://github.com/multiformats/js-multihashing) understands.

#### `marshal`

> returns a protobuf serialized version, compatible with go-ipfs MerkleDAG
Expand Down
17 changes: 10 additions & 7 deletions src/dag-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@

const protobuf = require('protocol-buffers')
const stable = require('stable')
const fs = require('fs')
const path = require('path')
const mh = require('multihashes')

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

const proto = protobuf(fs.readFileSync(path.join(__dirname, 'merkledag.proto')))
const proto = protobuf(require('./merkledag.proto'))
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 new.


function linkSort (a, b) {
return (new Buffer(a.name || '', 'ascii').compare(new Buffer(b.name || '', 'ascii')))
Expand Down Expand Up @@ -139,8 +137,8 @@ module.exports = class DAGNode {
}

// multihash - returns the multihash value of this DAGNode
multihash () {
this.encoded()
multihash (fn) {
this.encoded(fn)
return this._cached
}

Expand All @@ -157,12 +155,17 @@ module.exports = class DAGNode {

// Encoded returns the encoded raw data version of a Node instance.
// It may use a cached encoded version, unless the force flag is given.
encoded (force) {
encoded (fn, force) {
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 breaking API, (force, fn) wouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay will change

if (typeof fn === 'boolean') {
force = fn
fn = undefined
}

if (force || !this._encoded) {
this._encoded = this.marshal()

if (this._encoded) {
this._cached = util.hash(this._encoded)
this._cached = util.hash(this._encoded, fn)
}
}
return this._encoded
Expand Down
4 changes: 4 additions & 0 deletions src/merkledag.proto → src/merkledag.proto.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
'use strict'

module.exports = new Buffer(`
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 'ugly'.. Now I see how require worked.. this doesn't remove complexity, it even adds.

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 removes a lot of complexity for users, not using aegir. These proto files are the reason we need to use brfs as a preprocessor everywhere. If we drop it the code works out of the box in webpack and browserify.

Copy link
Member

Choose a reason for hiding this comment

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

Is this the only fs that required to be brfs'ified in js-ipfs-api? brfs'ified is a standard process in transpiling code to the browser anyway. This seems to me more of a way to postpone getting proper documentation that explains how to do this. Which we have to write anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only reason for using brfs for end users today is our use of fs.readFileSync to load proto files. We can avoid this by simply using a pattern like this. This means reduced build time for us and our users, in addition to having one less thing developers have to worry about. brfs is nice, but our end goal should be that webpack and browserify work on our modules without any additional configuration.

This is the first step for this :)

Copy link
Member

Choose a reason for hiding this comment

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

@dignifiedquire my main point is: we need to have a section in our main projects (js-ipfs, js-ipfs-api and js-libp2p) that explains our users how to 'webpack' all of this stuff, simply, it is better to have a walkthrough, even if it is obvious than having nothing. As an example, I don't believe that js-ipfs will ever stop needing a special webpack config.

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to have a section in our main projects (js-ipfs, js-ipfs-api and js-libp2p) that explains our users how to 'webpack' all of this stuff,

Yes, but we still should aim to make that explanation as simple as possible :)

I don't believe that js-ipfs will ever stop needing a special webpack config.

I have concrete plans for this and a roadmap in my head on how to get there, it is coming soon. Dropping fs.readFile is the first step on the road there.

// An IPFS MerkleDAG Link
message PBLink {

Expand All @@ -20,3 +23,4 @@ message PBNode {
// opaque user data
optional bytes Data = 1;
}
`)
11 changes: 9 additions & 2 deletions src/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,12 @@ const multihashing = require('multihashing')

exports = module.exports

// Hash is the global IPFS hash function. uses multihash SHA2_256, 256 bits
exports.hash = (data) => multihashing(data, 'sha2-256')
// Hash is the global IPFS hash function.
// Uses multihash SHA2_256, 256 bits as the default
exports.hash = (data, fn) => {
if (!fn) {
fn = 'sha2-256'
}

return multihashing(data, fn)
}
17 changes: 17 additions & 0 deletions test/dag-node-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const DAGNode = require('../src').DAGNode
const BlockService = require('ipfs-block-service')
const Block = require('ipfs-block')
const bs58 = require('bs58')
const mh = require('multihashes')

module.exports = function (repo) {
describe('DAGNode', function () {
Expand Down Expand Up @@ -57,6 +58,22 @@ module.exports = function (repo) {
done()
})

describe('multihash', () => {
it('defaults to sha2-256', () => {
const node = new DAGNode(new Buffer('4444'))
const res = mh.decode(node.multihash())

expect(res).to.have.property('name', 'sha2-256')
})

it('can use a different hash function', () => {
const node = new DAGNode(new Buffer('4444'))
const res = mh.decode(node.multihash('sha1'))

expect(res).to.have.property('name', 'sha1')
})
})

it('create a link', function (done) {
const buf = new Buffer('multihash of file.txt')
const link = new DAGLink('file.txt', 10, buf)
Expand Down