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

feat: integrate gossipsub by default #365

Merged
merged 5 commits into from
Jul 31, 2019

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented May 24, 2019

BREAKING CHANGE: new configuration for deciding the implementation of pubsub to be used.
In this context, the experimental flags were also removed.

This PR modified the configuration of a libp2p node, in order to allow the selection of the pubsub implementation to use. Also Gossipsub was added as the default pubsub implementation in the tests

Needs:

Unblocks:

@vasco-santos vasco-santos changed the title feat: integrate gossipsub by default [WIP] feat: integrate gossipsub by default May 24, 2019
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch 2 times, most recently from 3fc3adc to e753125 Compare May 24, 2019 16:22
@vasco-santos
Copy link
Member Author

cc @wemeetagain @Mikerah

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.

Just some minor comments, but overall this looks good.

test/pubsub.node.js Outdated Show resolved Hide resolved
test/pubsub.node.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 4edf353 to 4dd2e46 Compare July 11, 2019 13:53
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 913c809 to 34f663a Compare July 11, 2019 17:52
src/index.js Outdated Show resolved Hide resolved
@jacobheun jacobheun force-pushed the feat/integrate-gossipsub-by-default branch from 9c48b1b to 2b76101 Compare July 12, 2019 12:15
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch 3 times, most recently from 1dd19c2 to 3790ca8 Compare July 18, 2019 13:35
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.

1 usability thing with configuration, otherwise I like where this is at.

src/index.js Outdated
if (this._config.EXPERIMENTAL.pubsub) {
this.pubsub = pubsub(this)
// start pubsub
if (this._config.pubsub.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vasco-santos what do you think about changing this logic a bit. I think if this._modules.pubsub is set, and this._config.pubsub.enabled !== false we should use pubsub. This would avoid people needing to add and enable it. We do this for discovery. If you've configured it, there's a really good chance you want to use 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.

I agree, this is a better dev ex

@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from e45b1bb to c2cd663 Compare July 26, 2019 08:13
@jacobheun jacobheun mentioned this pull request Jul 29, 2019
19 tasks
@vasco-santos vasco-santos changed the title [WIP] feat: integrate gossipsub by default feat: integrate gossipsub by default Jul 30, 2019
BREAKING CHANGE: new configuration for deciding the implementation of pubsub to be used.
In this context, the experimental flags were also removed.
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from c5be8dd to 65866b2 Compare July 30, 2019 20:30
@vasco-santos
Copy link
Member Author

@jacobheun gossipsub was released 🎉

Just rebased and updated the PR

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.

Woot! I'll get an rc cut.

@jacobheun jacobheun merged commit 791f39a into master Jul 31, 2019
@jacobheun jacobheun deleted the feat/integrate-gossipsub-by-default branch July 31, 2019 07:38
@jacobheun
Copy link
Contributor

0.26.0-rc.0 is out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants