Eslint config - replace common-style with eslint-config-populist #744
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed some discussion about outdated npm packages causing audit warnings on install and one culprit was common-style package - which is now deprecated in favour of eslint-config-populist which itself hasn't been updated for 3 years, but at least it's not vulnerable.
This change removes the critical
npm audit
issue and most of the high issues. The remaining package causing the 1 high issue is an older version of lodash which is a dependency for tap. Upgrading tap will produce breaking changes and require updates to tests, so I will tackle that separately.Results of
npm audit
Before
31 vulnerabilities (19 moderate, 11 high, 1 critical)
After
23 vulnerabilities (22 moderate, 1 high)
What changes did you make?
I have removed common-style and added the eslint-config-populist package and eslint to dev dependencies.
Rather than make a lot of changes or apply opinionated rules that contradict a lot of the established code, I tried to pick off minimal impact changes such as curly braces and quotes, unused vars, etc.
For everything else I've either explicitly excluded it or marked rules with many violations as warning only.
Rules configured as warnings:
I made an exception for the following bits of code (explicitly disabled with comment):
lib/http-server.js - the alternative here would require a bit of refactoring
lib/http-server.js - this seems like an ok use of the ternary operator to me, refactoring would introduce extra vars or functions
Provide some example code that this change will affect, if applicable:
This shouldn't affect any existing code, but should affect future code consistency.
Is there anything you'd like reviewers to focus on?
My goal here was to introduce eslint without making a ton of unnecessary and/or opinionated changes. I chose the config that should have otherwise been applied via the previous common-style package and was rather conservative with the changes I made to code.
Having said all that, I think it may be appropriate in the future to move to a more actively maintained style config and enforce rather than warn for more consistency and better standards in the code base.
Please ensure that your pull request fulfills these requirements:
master
branchhttp-server --help
textI didn't see an appropriate place to mention this in the README as it seems to be centered around usage rather than development, but open to feedback on this.
What is the purpose of this pull request? (bug fix, enhancement, new feature,...)
It's kind of both a bug fix and an enhancement.
The original discussions around this have unfortunately been closed and I cannot find them.