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

feat/use new bitswap #51

Merged
merged 2 commits into from Dec 23, 2016
Merged

feat/use new bitswap #51

merged 2 commits into from Dec 23, 2016

Conversation

daviddias
Copy link
Member

@daviddias daviddias added the status/in-progress In progress label Dec 19, 2016
@daviddias daviddias self-assigned this Dec 19, 2016
@@ -21,7 +21,7 @@ module.exports = class BlockService {
}

isOnline () {
return this._bitswap != null
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.

please don't, != checks for both null and undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Well.. you are always setting it up to null, if it happens to be undefined then probably something weird happened, but ok, I can revert

} else {
ps = this._repo.blockstore.putStream()
return pull(
pull.map((blockAndCID) => {
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to change this, currently we are going from having a Block, to this strange object, and inside bitswap back to a Block. Can we please make the Block interface sufficient so we can pass it through the whole stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, BlockService is blockAndCID and now bitswap is also blockAndCID, because bitswap needs to know CIDs, where do we in Bitswap move it to just Block?

Copy link
Member

Choose a reason for hiding this comment

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

looks like the old inefficiency in bitswap is gone with the refactor, then I'm fine with this :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, I would love to have something nicer, the blockAndCID isn't my favourite, but we really can't use something as Block right now unless we 'patch' it every time to have the right CID, which won't happen when reading from the repo, and there the only identifier is the multihash. Right now, Block is really just a buffer + a convenience method that invokes multihashing, it might make sense to do what @kumavis brought up in the last call and just remove that type and do { data: cid: } objects all the way. (Still not convinced of what is simpler)

Copy link

@kumavis kumavis Dec 19, 2016

Choose a reason for hiding this comment

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

yeah ipfs-block should just wrap { data, cid }
ipld/js-ipld-block#25

Copy link
Member

Choose a reason for hiding this comment

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

I think with the info about the repo changes in go-ipfs we will have to rethink this anway.

@daviddias daviddias merged commit f64441c into master Dec 23, 2016
@daviddias daviddias deleted the feat/use-new-bitswap branch December 23, 2016 08:54
@daviddias daviddias removed the status/in-progress In progress label Dec 23, 2016
@dignifiedquire
Copy link
Member

🎉

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.

None yet

3 participants