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

Introduce web3 request batching #3

Merged
merged 6 commits into from
Dec 18, 2018
Merged

Introduce web3 request batching #3

merged 6 commits into from
Dec 18, 2018

Conversation

kelsos
Copy link
Contributor

@kelsos kelsos commented Nov 29, 2018

  • Update to web3.js 1.0.0
  • Create BatchManager class to handle request batching.
  • Add tests for the mechanism

Closes #9

@@ -0,0 +1,13 @@
const fs = require('fs');
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be tricky, you're trusting on path consistency and changing things in distribution files. Isn't there really other way to change this specific config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was looking into the issue angular/angular-cli#1548, I chose this as the most viable of the proposed solutions, some others required some extra third party dependencies that I would rather avoid using.

Since then I didn't really look into it since it was working. I will have to go through the thread once again and see if there is a better solution or if this has been fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there is no upstream fix for these it seems, I started getting the error as soon as I removed the patch.js from running.

I won't disagree that it is tricky, however this webpack configuration is hidden inside angular's and at least from what I understood, you are not supposed to customize it, so there is no way to change it other than patching it.

The reason that I chose this approach is that it was the only solution that didn't require installing extra thirdparty dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Worst case you could eject, but I think it's not worth the trouble right now, seems like this is affecting enough people to be fixed quite soon, so as a temporary workaround could stay, as soon as you include specific notes and TODOs linking the issue in question. Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comment to the patch. Is that ok ?

@codecov-io
Copy link

Codecov Report

Merging #3 into master will increase coverage by 2.15%.
The diff coverage is 66.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage    58.1%   60.26%   +2.15%     
==========================================
  Files          38       40       +2     
  Lines        1363     1510     +147     
  Branches      267      308      +41     
==========================================
+ Hits          792      910     +118     
+ Misses        362      361       -1     
- Partials      209      239      +30
Impacted Files Coverage Δ
...nents/payment-history/payment-history.component.ts 89.39% <100%> (+0.16%) ⬆️
...twork-selector/token-network-selector.component.ts 85.5% <100%> (ø) ⬆️
src/app/services/raiden.config.ts 31.42% <13.33%> (+0.39%) ⬆️
src/app/services/raiden.service.ts 45.5% <54.28%> (-3%) ⬇️
src/app/services/token-info-retriever.service.ts 70.12% <66.66%> (ø)
src/app/services/batch-manager.ts 81.25% <81.08%> (ø)
... and 4 more

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 4675ece...a935cdb. Read the comment docs.

@kelsos kelsos merged commit d7c2db3 into raiden-network:master Dec 18, 2018
@kelsos kelsos deleted the web3-batching branch December 18, 2018 11:05
@kelsos kelsos mentioned this pull request Dec 21, 2018
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.

3 participants