Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: gossipsub as default pubsub #2298

Merged
merged 11 commits into from
Sep 2, 2019
Merged

Conversation

vasco-santos
Copy link
Member

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

Needs:

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch 6 times, most recently from 448bfed to 6a47db6 Compare July 26, 2019 13:26
@jacobheun
Copy link
Contributor

@vasco-santos fyi, I pushed up a version bump to the new libp2p RC and also fixed the pubsub methods to correct the breaking changes.

@jacobheun
Copy link
Contributor

Since we've changed how pubsub is registered, we'll need to release this with the update of libp2p

@vasco-santos
Copy link
Member Author

Thanks @jacobheun

@vasco-santos
Copy link
Member Author

vasco-santos commented Jul 31, 2019

We also need to change the gossipsub version and update the bundle size. I will do it

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch from 0315c72 to b66463b Compare July 31, 2019 08:26
@jacobheun
Copy link
Contributor

I missed 1 subscribe call, that's been corrected. It's 🍏 !

@vasco-santos
Copy link
Member Author

vasco-santos commented Jul 31, 2019

thanks for looking into this @jacobheun

@alanshaw this is ready for you to have a look
also @jacobheun if you want to check the config part of the things in the PR, if you did not

@jacobheun jacobheun force-pushed the feat/gossipsub-as-default-pubsub branch 2 times, most recently from 19573fe to e99c57d Compare August 9, 2019 10:51
@jacobheun jacobheun marked this pull request as ready for review August 19, 2019 16:54
@jacobheun jacobheun requested a review from alanshaw August 19, 2019 16:54
@jacobheun jacobheun force-pushed the feat/gossipsub-as-default-pubsub branch from e99c57d to 7b9d66f Compare August 21, 2019 17:58
@alanshaw alanshaw mentioned this pull request Aug 27, 2019
66 tasks
doc/config.md Show resolved Hide resolved
package.json Show resolved Hide resolved
src/core/components/libp2p.js Outdated Show resolved Hide resolved
src/core/components/pubsub.js Show resolved Hide resolved
src/core/components/pubsub.js Show resolved Hide resolved
src/core/config.js Show resolved Hide resolved
src/core/runtime/config-browser.js Outdated Show resolved Hide resolved
src/core/runtime/config-nodejs.js Show resolved Hide resolved
test/core/libp2p.spec.js Show resolved Hide resolved
@alanshaw
Copy link
Member

@vasco-santos interface-ipfs-core@0.111 has been released.

.aegir.js Outdated
@@ -8,7 +8,7 @@ const ipfsdServer = IPFSFactory.createServer()
const preloadNode = MockPreloadNode.createNode()

module.exports = {
bundlesize: { maxSize: '689kB' },
bundlesize: { maxSize: '694kB' },
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully this can stay the same if we're not bundling 2 implementations?

@alanshaw
Copy link
Member

FYI I have updated ipfs-inactive/js-ipfs-http-client#1059 to support string data in pubsub.publish so don't worry about sending a separate PR!

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch from 7b9d66f to aa2eb36 Compare August 28, 2019 12:26
README.md Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch from aa2eb36 to 8f3dcaa Compare August 28, 2019 12:50
@vasco-santos
Copy link
Member Author

@alanshaw I think that I addressed everything from your review

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

There still seems to be references to EXPERIMENTAL.pubsub in the code (including the examples). Can you make sure these are all fixed?

package.json Outdated
"interface-ipfs-core": "^0.110.0",
"ipfsd-ctl": "^0.44.1",
"interface-ipfs-core": "^0.111.0",
"ipfsd-ctl": "ipfs/js-ipfsd-ctl#fix/pubsub-flag-defaults",
Copy link
Member Author

@vasco-santos vasco-santos Aug 31, 2019

Choose a reason for hiding this comment

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

@alanshaw @hugomrdias we have a circular dependency issue here with ipfsd-ctl:

ipfs/js-ipfsd-ctl#363

Copy link
Member

Choose a reason for hiding this comment

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

Why not just have --enable-pubsub as the official option name and create an alias as --enable-pubsub-experiment?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, are you recommending to alias here, and use in the tests --enable-pubsub-experiment for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

should i merge/release that PR as is ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do, we cannot delete this branch. But it is the only way to get CI green. My main concern is that we need to wait for the js-ipfs release 😡

Copy link
Member

Choose a reason for hiding this comment

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

we can point to master or a commit until the release

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 am ok with merging that, and take the AI of PR ctl to use master once this PR is merged

Copy link
Member

Choose a reason for hiding this comment

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

cool

Copy link
Member

Choose a reason for hiding this comment

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

ctl released as 0.45.0

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch 2 times, most recently from a30d22d to c21d362 Compare August 31, 2019 13:57
@vasco-santos
Copy link
Member Author

Also, I am having issues with dns tests in browser. The same is occurring in master though

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch 2 times, most recently from 2e44237 to 8f56488 Compare September 2, 2019 17:00
@alanshaw
Copy link
Member

alanshaw commented Sep 2, 2019

@vasco-santos I had to revert the upgrade of ipfs-http-client in master because it allows string messages so you'll need to update it here.

@vasco-santos vasco-santos force-pushed the feat/gossipsub-as-default-pubsub branch from 8f56488 to 60c732a Compare September 2, 2019 18:17
@vasco-santos
Copy link
Member Author

As talked with @alanshaw , I skipped the dns tests which are currently failing due to a gateway issue (not related to this). I will create a new PR to enable them as soon as this is merged

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