-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Improve clarity of HttpProvider keepAlive value #3463
Conversation
The ideal version of this may be |
That seems clear and straight-forward to me if you want to update the PR to that :) Thanks @gnidan for your feedback about the default behavior. @prestwich it might be helpful to others in the future too if you want to add a small comment in the source there (about the default behavior of keepAlive). |
Updated the PR, added a comment, and squashed to a single commit |
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.
LGTM! thanks :)
Edit: actually sorry one last thing, could you add an entry in the changelog noting the update?
Updated the changelog in a7c6156 |
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.
LGTM
Don't use a ternary statement on a boolean statement to resolve truthiness. Just double-negate it
Description
Please include a summary of the change and be sure you follow the contributions rules we do provide here
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:cov
and my test cases do cover all lines and branches of the added code.npm run build-all
and tested the resulting file/'s fromdist
folder in a browser.CHANGELOG.md
file in the root folder.