-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
448bfed
to
6a47db6
Compare
@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. |
Since we've changed how pubsub is registered, we'll need to release this with the update of libp2p |
Thanks @jacobheun |
We also need to change the gossipsub version and update the bundle size. I will do it |
0315c72
to
b66463b
Compare
I missed 1 subscribe call, that's been corrected. It's 🍏 ! |
thanks for looking into this @jacobheun @alanshaw this is ready for you to have a look |
19573fe
to
e99c57d
Compare
e99c57d
to
7b9d66f
Compare
@vasco-santos |
.aegir.js
Outdated
@@ -8,7 +8,7 @@ const ipfsdServer = IPFSFactory.createServer() | |||
const preloadNode = MockPreloadNode.createNode() | |||
|
|||
module.exports = { | |||
bundlesize: { maxSize: '689kB' }, | |||
bundlesize: { maxSize: '694kB' }, |
There was a problem hiding this comment.
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?
FYI I have updated ipfs-inactive/js-ipfs-http-client#1059 to support string data in |
7b9d66f
to
aa2eb36
Compare
aa2eb36
to
8f3dcaa
Compare
@alanshaw I think that I addressed everything from your review |
There was a problem hiding this 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?
8f3dcaa
to
011827d
Compare
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", |
There was a problem hiding this comment.
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
:
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I need the ctl to do: https://github.com/ipfs/js-ipfsd-ctl/pull/363/files#diff-d06a3c390787c7b35d2ab649265df1bbR50
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 😡
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
There was a problem hiding this comment.
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
a30d22d
to
c21d362
Compare
Also, I am having issues with dns tests in browser. The same is occurring in master though |
2e44237
to
8f56488
Compare
@vasco-santos I had to revert the upgrade of |
8f56488
to
60c732a
Compare
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 |
Needs:
js-libp2p
with libp2p/js-libp2p#365