-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
@@ -0,0 +1,13 @@ | |||
const fs = require('fs'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
BatchManager
class to handle request batching.Closes #9