Skip to content
This repository was archived by the owner on Dec 6, 2022. It is now read-only.

Conversation

@dignifiedquire
Copy link
Member

BREAKING CHANGE:
Constructor requires a hash now and there is now Block.create(data, type, cb)

README.md Outdated

Creates a new block with raw data `data`. `type` can be either `'protobuf'` or `'ipld'`.

#### `Block.create(data, key, [type, ] callback)`
Copy link
Member

Choose a reason for hiding this comment

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

Let's just go ahead and take from #5 the fact that .key becomes a function and make it an async function.

Copy link
Member Author

Choose a reason for hiding this comment

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

@diasdavid done, please review the new api before I start propagating

package.json Outdated
},
"homepage": "https://github.com/ipfs/js-ipfs-block#readme",
"devDependencies": {
"aegir": "^8.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

update to latest

Copy link
Member

Choose a reason for hiding this comment

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

🛎

@daviddias
Copy link
Member

daviddias commented Oct 12, 2016

API looks good, needs:

  • a test that tests with a different hash func

@daviddias daviddias changed the title fix: transition to async crypto api Async Crypto Endeavour Oct 30, 2016
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Overall LGTM

README.md Outdated
- [`block.data`](#blockdata)
- [`block.key`](#blockkey)
- [`block.extension`](#blockextension)
- [`block.key([hashFn,] callback)`](#blockkeyhashfn-callback)
Copy link
Member

Choose a reason for hiding this comment

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

s/hashFn/hashAlg

README.md Outdated

The [multihash][multihash] of the block's data, as a buffer.

#### `block.key([hashFn,] callback)`
Copy link
Member

Choose a reason for hiding this comment

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

s/hashFn/hashAlg

set () {
throw new Error('Tried to change an immutable block')
}
})
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/index.js Outdated

multihashing(this.data, hashFunc, callback)
if (this._cache[hashFunc]) {
setImmediate(() => {
Copy link

Choose a reason for hiding this comment

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

setImmediate isn't supported by FF or Chrome, will it be a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm thanks for pointing this out, need to find a better solution :(

Copy link

Choose a reason for hiding this comment

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

setImmediate is also used in 4 different places across the codebase. There is also polyfill for it if it comes to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we were shipping one before, but not anymore. As we already use async I'm going to use async.setImmediate http://caolan.github.io/async/docs.html#setImmediate

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@daviddias daviddias merged commit bb49479 into master Nov 3, 2016
@daviddias daviddias deleted the webcrypto branch November 3, 2016 12:40
@daviddias daviddias removed the status/in-progress In progress label Nov 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants