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

Create client module from bclient #838

Merged
merged 1 commit into from Sep 13, 2019

Conversation

@braydonf
Copy link
Contributor

braydonf commented Sep 6, 2019

Closes #834

@codecov-io

This comment was marked as outdated.

Copy link

codecov-io commented Sep 6, 2019

Codecov Report

Merging #838 into master will decrease coverage by 0.14%.
The diff coverage is 41.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
- Coverage   62.09%   61.94%   -0.15%     
==========================================
  Files         147      150       +3     
  Lines       25392    25574     +182     
==========================================
+ Hits        15766    15842      +76     
- Misses       9626     9732     +106
Impacted Files Coverage Δ
lib/wallet/client.js 0% <0%> (ø) ⬆️
lib/bcoin.js 0% <0%> (ø) ⬆️
lib/bcoin-browser.js 0% <0%> (ø) ⬆️
lib/client/index.js 100% <100%> (ø)
lib/client/node.js 27.65% <27.65%> (ø)
lib/client/wallet.js 48.03% <48.03%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7572246...f79224f. Read the comment docs.

@tynes

This comment has been minimized.

Copy link
Member

tynes commented Sep 6, 2019

NACK until there is a clear upgrade path for users of just bclient itself. @BluSyn thoughts?

This change will blow up the size of the bundle for anybody that wants just the client since bcoin doesn't support es modules (with tree shaking) yet.

@braydonf

This comment has been minimized.

Copy link
Contributor Author

braydonf commented Sep 7, 2019

@tynes This adds the client to bcoin so that it can move with tightly coupled parts within wallet. I would imagine other applications, similar to wallet, that depend on bclient would have a similar need and would do something similar. However, this is not removing bclient, which can still continue to be used, as well as bcurl.

@tynes

This comment has been minimized.

Copy link
Member

tynes commented Sep 9, 2019

ACK on the agreement that any changes to the bclient codebase that is brought into bcoin will be done in a way that makes it easy to backport the changes to the bclient package. This will prevent users of bclient from falling out of compatibility.

Its likely there won't be a ton of changes over time, so this should be manageable to do.

braydonf added a commit that referenced this pull request Sep 13, 2019
Create client module from bclient
@braydonf braydonf merged commit f79224f into bcoin-org:master Sep 13, 2019
3 checks passed
3 checks passed
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@braydonf braydonf deleted the braydonf:client branch Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.