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/async await #202

Merged
merged 15 commits into from
Sep 24, 2019
Merged

Feat/async await #202

merged 15 commits into from
Sep 24, 2019

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jul 1, 2019

Supersedes #192

This PR depends on a number of async/await PRs that are still WIP (peer-id, peer-info, etc) so it is still some way from being ready.

Still pending PeerId and PeerInfo async/await - they've been done but
we can't use them until libp2p update libp2p-secio so use promisify
in the interim where necessary, which is only in the tests.
@achingbrain achingbrain marked this pull request as ready for review September 20, 2019 10:02
src/decision-engine/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/network.js Outdated Show resolved Hide resolved
src/network.js Outdated Show resolved Hide resolved
src/network.js Outdated Show resolved Hide resolved
src/types/message/index.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/network.js Outdated Show resolved Hide resolved
src/network.js Outdated Show resolved Hide resolved
Co-Authored-By: dirkmc <dirkmdev@gmail.com>
@dirkmc
Copy link
Contributor Author

dirkmc commented Sep 23, 2019

It makes me a bit nervous that the tests were passing when the protocols were switched around. @achingbrain do you think it's worth adding a test to catch that case?

@achingbrain
Copy link
Member

It worked because all nodes in the tests are JS nodes and the message deserialization code handles both message versions by checking to see if the relevant fields exist.

I guess we could add a 'strict' mode that only processes one version of bitswap but that sounds beyond the scope of this PR.

@dirkmc
Copy link
Contributor Author

dirkmc commented Sep 24, 2019

Ok fair enough - I can't officially approve as I opened the PR, but LGTM 👍

Let me know if and when you want me to merge + release

@achingbrain
Copy link
Member

Yes please, whenever you can :)

@dirkmc dirkmc merged commit accf53b into master Sep 24, 2019
@dirkmc dirkmc deleted the feat/async-await branch September 24, 2019 16:12
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.

4 participants