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

Upgrade to new block and datastore interface #58

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Upgrade to new block and datastore interface #58

merged 2 commits into from
Mar 21, 2017

Conversation

dignifiedquire
Copy link
Member

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.

  • rm yarn.lock

I see that this is introducing more than an update to how it uses its deps, it is also introducing a whole new API that removes pull-streams from the equation. I feel it is kind of simpler this way, but at the same time, I need to ask know why the change, we can't go back and forth with our APIs, or is there a good reasoning to drop pull-streams at this level, or simply leave it there.


// setup a repo
var repo = new IPFSRepo('example', { stores: Store })
const repo = new IPFSRepo('example')
Copy link
Member

Choose a reason for hiding this comment

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

s/'example'/<path>

README.md Outdated
console.log(block.data.toString() === b.data.toString())
const data = new Buffer('hello world')
multihashing(data, 'sha2-256', (err, hash) => {
if (err) throw err
Copy link
Member

Choose a reason for hiding this comment

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

{}

README.md Outdated
bs.get(cid, function (err, b) {
console.log(block.data.toString() === b.data.toString())
const data = new Buffer('hello world')
multihashing(data, 'sha2-256', (err, hash) => {
Copy link
Member

Choose a reason for hiding this comment

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

s/hash/multihash

README.md Outdated
multihashing(data, 'sha2-256', (err, hash) => {
if (err) throw err

const block = new Block(data, new CID(hash))
Copy link
Member

Choose a reason for hiding this comment

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

make the cid a variable

README.md Outdated
// add the block, then retrieve it
bs.put(block, (err) => {
if (err) throw err
bs.get(block.cid, (err, b) => {
Copy link
Member

Choose a reason for hiding this comment

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

and use the cid from the var, just to make it obvious that you don't need to get the cid from the block.

README.md Outdated
bs.put(block, (err) => {
if (err) throw err
bs.get(block.cid, (err, b) => {
if (err) throw err
Copy link
Member

Choose a reason for hiding this comment

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

{}

*
* @param {Bitswap} bitswap
* @returns {void}
*/
goOnline (bitswap) {
this._bitswap = bitswap
}
Copy link
Member

Choose a reason for hiding this comment

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

This method name is a bit misleading. By passing bitswap, we are not necessarily going online (i.e bitswap is offline).

What about addExchange, so that in the future, we even just have to add multiple exchanges and it all works.

Copy link
Member Author

Choose a reason for hiding this comment

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

break all the apis

Copy link
Member

Choose a reason for hiding this comment

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

We are doing it already..

* Is the blockservice online, i.e. is bitswap present.
*
* @returns {bool}
*/
isOnline () {
return this._bitswap != null
}
Copy link
Member

Choose a reason for hiding this comment

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

if above is applied, then this would be: hasExchange

Copy link
Member

Choose a reason for hiding this comment

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

@dignifiedquire did you get this comment?

Copy link
Member

Choose a reason for hiding this comment

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

Deferring for discussion #61

* @param {function(Error)} callback
* @returns {void}
*/
putMany (blocks, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

wait, why not pullStream based anymore?

expect(err).to.not.exist
const cid = new CID(key)
const data = new Buffer('A random data block')
multihashing(data, 'sha2-256', (err, hash) => {
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of our users, it is better to call the thing that comes out of multihashing a multihash because that is what it is.

@dignifiedquire
Copy link
Member Author

I see that this is introducing more than an update to how it uses its deps, it is also introducing a whole new API that removes pull-streams from the equation. I feel it is kind of simpler this way, but at the same time, I need to ask know why the change, we can't go back and forth with our APIs, or is there a good reasoning to drop pull-streams at this level, or simply leave it there.

So pull-streams are cool, if you need stream. These apis were a propagation of the underlying blob-store interfaces and eventually I realised that they weren't helping anyone. This is why datastore only has pull-streams for the query interface and regular callback based apis for everything else.
I agree we shouldn't change apis all the time, but this change I feel makes it easier to use and implement in many places (so many pull()s I could remove that were doing just what a single callback call could have done). I know you argued for the non stream based api before and I'm sorry it took me a while to realise that you were right all along <3

@daviddias daviddias merged commit 353f0e4 into master Mar 21, 2017
@daviddias daviddias deleted the next branch March 21, 2017 07:27
@daviddias daviddias removed the status/in-progress In progress label Mar 21, 2017
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.

2 participants