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

Async Refactor: providers #133

Merged
merged 4 commits into from
Jun 10, 2019
Merged

Async Refactor: providers #133

merged 4 commits into from
Jun 10, 2019

Conversation

kumavis
Copy link
Contributor

@kumavis kumavis commented Jun 3, 2019

providers.js hasn't been touched in a while, so this is mostly directly taken from #82

Added some async/callback wrappers here and there and used pull-stream-to-async-iterator around datastore.query as #82 originally relied on the new datastore interface

Fixes #124

Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

I suggested a couple of small changes. Is there a reason for adding async to methods that already return Promises? If I understand correctly this will just add an extra layer of Promises without any functional change in the code?

src/index.js Show resolved Hide resolved
src/providers.js Show resolved Hide resolved
src/providers.js Show resolved Hide resolved
src/providers.js Show resolved Hide resolved
src/providers.js Show resolved Hide resolved
src/providers.js Show resolved Hide resolved
test/rpc/handlers/get-providers.spec.js Show resolved Hide resolved
@kumavis
Copy link
Contributor Author

kumavis commented Jun 3, 2019

@dirkmc committed the comment changes
The other points of feedback refer intentional to decisions I made, and I responded to the first instance of each

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.

This looks good @kumavis thanks!

Just two minor requests

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

Also, could you increase the bundlesize max size in .aegir, in order to have a green CI?

@kumavis kumavis force-pushed the feat/async-providers branch from 05e1500 to c5e9a9a Compare June 9, 2019 15:44
@kumavis
Copy link
Contributor Author

kumavis commented Jun 9, 2019

@vasco-santos @jacobheun ready to go

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! 🚀

@vasco-santos vasco-santos merged commit 30fd44a into master Jun 10, 2019
@vasco-santos vasco-santos deleted the feat/async-providers branch June 10, 2019 09:44
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.

dht._findProvidersSingle returns duplicate providers
3 participants