-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Add flag for toggling permessage-deflate #3286
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## main #3286 +/- ## ========================================== + Coverage 54.78% 57.54% +2.76% ========================================== Files 23 24 +1 Lines 1265 1279 +14 Branches 286 290 +4 ========================================== + Hits 693 736 +43 + Misses 460 441 -19 + Partials 112 102 -10
Continue to review full report at Codecov.
|
increase coverage by 0.12%
👏 You + Codecov helping us build a habit of increasing code coverage!
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.
Overall, great work! 🎉 Just a couple nits about functions being pure
2889725 to
6ff495e
Compare
1da4d67 to
fd4b784
Compare
This is so they can be unit tested.
All it does is log and exit which is what the caller will be doing on an error anyway (see entry).
e06725d to
a882be5
Compare
Refactored; it's not any more pure but it's simpler. I think we could have an interesting discussion on whether we should pass args (and other variables) around instead of using a global access pattern at some point.
Coverage went down because of the code added in entry so I split some of those functions out in order to test them. One of them is now used for all the integration tests so those should reflect reality more closely now.
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.
Great work 👏
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 looks way cleaner/easier to read 👏
@jawnsy
jawnsy
May 5, 2021
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.
👍 yea, the code before was 🤨 kinda inscrutable, but then again, I'm not a JavaScript/TypeScript person
I think we could have an interesting discussion on whether we should pass args (and other variables) around instead of using a global access pattern at some point
Agreed! I prefer passing args because it's easier to mock/test things. But global access does come in handy when writing code. It would be a good discussion though.
I'm going to fix the trivy scan in a PR soon so you can ignore for now
Uh oh!
There was an error while loading. Please reload this page.
This is a workaround for #3014 until we figure it out (if it is indeed related which it might not be; the zlib errors could merely be side effects of a connection issue).