Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

refactor: async #26

Merged
merged 6 commits into from
Nov 14, 2019
Merged

refactor: async #26

merged 6 commits into from
Nov 14, 2019

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Aug 30, 2019

BREAKING CHANGE: Switch to using async/await and async iterators.

Needs:

BREAKING CHANGE: Switch to using async/await and async iterators.
@vasco-santos
Copy link
Member Author

@jacobheun per our discussion earlier, I made the first iteration on the proposal for extracting the need for the libp2p instance in pubsub system.

Moreover, the dialing layer within pubsub/dht/... is intended to be removed, as we are currently duplicating a lot of dialing management within subsystems, like tracking dials and having dial queues, which could be controlled by js-libp2p / js-libp2p-connection-mgr.

https://gist.github.com/vasco-santos/2c39ab16d15a87c573bef4f916472831

I am not particularly convinced about the register side of things yet. First, I think we can have a better naming. Then, we can add other option features like autoDial (which I am going with default true right now), and connection management options (the subsystem can communicate js-libp2p meaningful numbers like the min/max number of connection in that protocol that it would like to have).

@vasco-santos vasco-santos force-pushed the refactor/async branch 3 times, most recently from 6cf5a47 to 5e164c6 Compare October 15, 2019 10:42
src/index.js Outdated
* @param {function} registrar.unregister
* @param {Object} [options]
* @param {boolean} [options.signMessages] if messages should be signed, defaults to true
* @param {boolean} [options.strictSigning] if message signing should be required, defaults to true
Copy link
Member Author

Choose a reason for hiding this comment

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

My idea here is to add the topology to the options, which would overwrite the ones defined by the protocol. Should we do it here, or iterate the registrar topology in a following PR? My main concern is to add docs to stuff that we will not support yet

src/peer.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Member Author

@jacobheun Already migrated this and did the same for floodsub and gossipsub. I will be working on the next couple of days of the PR to integrate this in js-libp2p

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/instance.spec.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
}
```

## API

The following specified API should be the base API for a pubsub implementation on top of `libp2p`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow this statement, is it necessary? How does this API related to https://github.com/ipfs/interface-js-ipfs-core/blob/master/SPEC/PUBSUB.md ?

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 is not necessary. But, taking into account that we want to remove the pubsub function mapping from js-libp2p, and just keep it's instance there, all the implementations should provide the same API.

Regarding the ipfs spec one, I don't think the ls and peers API is understandable, and we already updated those in libp2p-daemon/libp2p-daemon-client. They will have to get that updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now libp2p points to that API for docs, https://github.com/libp2p/js-libp2p#libp2ppubsub. We should either get the ipfs api corrected, or if we're looking to have a different API for libp2p, make sure we have appropriate doc references.

Copy link
Member Author

Choose a reason for hiding this comment

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

Being worked on libp2p/js-libp2p#467 and will also be pointed in this repo afterward

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Outdated Show resolved Hide resolved
test/pubsub.spec.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the refactor/async branch 2 times, most recently from a49d632 to 0b67775 Compare October 29, 2019 16:21
@vasco-santos vasco-santos force-pushed the refactor/async branch 6 times, most recently from bbc18b7 to dc0b982 Compare October 30, 2019 16:45
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Overall this looks good, I noticed there aren't any tests yet for the new getPeersSubscribed. It might be good to add some to make sure we're getting back what we expect.

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

🚢 🚀

@vasco-santos vasco-santos merged commit c690b29 into master Nov 14, 2019
@vasco-santos vasco-santos deleted the refactor/async branch November 14, 2019 13:38
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