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

#92: use callback in start from js-libp2p #93

Merged
merged 3 commits into from
Jul 31, 2019

Conversation

generalpiston
Copy link
Contributor

No description provided.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @generalpiston

Asked you for a simple change

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

also @generalpiston Can you please rebase this PR with master?

@generalpiston
Copy link
Contributor Author

Done

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR @generalpiston

@vasco-santos vasco-santos merged commit 74c305d into libp2p:master Jul 31, 2019
@alanshaw
Copy link
Member

alanshaw commented Aug 1, 2019

@vasco-santos this repo has already been converted to async/await. There should be no callbacks here. If there's bugfixes that are needed in the callback version then they need to be backported to a v0.9.x branch.

@vasco-santos
Copy link
Member

you are right @alanshaw !

I completely missed that this module was already converted. I will revert this commit!

@generalpiston libp2p-bootstrap@0.9.x already includes the callback in start. We are currently migrating all the modules to async/await, as while this is a WIP some latest versions might not be in use. In this context, libp2p is still using 0.9.7, which uses the callback implementation. Once some other dependencies get converted, and libp2p will upgrade those dependencies and will be refactored accordingly!

vasco-santos added a commit that referenced this pull request Aug 1, 2019
@generalpiston
Copy link
Contributor Author

@vasco-santos that makes perfect sense. Should we update the package.json peerDependencies to use a newer version of js-libp2p? Or add a note to the README?

@vasco-santos
Copy link
Member

I created a PR on js-libp2p to warn people about this.

Thanks @generalpiston

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.

3 participants