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

Eslint config - replace common-style with eslint-config-populist #744

Merged
merged 3 commits into from Oct 15, 2021

Conversation

chris--jones
Copy link
Contributor

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

      // eslint-disable-next-line no-sync
      fs.lstatSync('./public');

lib/http-server.js - this seems like an ok use of the ternary operator to me, refactoring would introduce extra vars or functions

    // eslint-disable-next-line no-nested-ternary
    options.cache === undefined ? 3600 :
    // -1 is a special case to turn off caching.
    // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#Preventing_caching
    options.cache === -1 ? 'no-cache, no-store, must-revalidate' :
    options.cache // in seconds.

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:

  • The pull request is being made against the master branch
  • [] Tests for the changes have been added (for bug fixes / features) - not applicable
  • New features/options have been documented in:
    • http-server --help text
    • README.md
    • doc/http-server.1

I 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.

add eslint config and dev dependency
apply a few small eslint fixes
@thornjad thornjad added the patch version Small, non-breaking, bug fix or trivial change label Oct 13, 2021
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

This is excellent! I really appreciate your approach of using a little more up-to-date style without imposing lots of new opinions! I definitely want to move to a more current style (my current go-to is semistandard, but I could be swayed), and that will be much easier after this change. Thank you!

I fully expect tests to pass but I'll wait for them before merging

@thornjad
Copy link
Member

Oh looks like there's a package-lock conflict, if you merge master in, delete the lock and recreate it, that should be fine

@thornjad thornjad added the dependencies Pull requests that update a dependency file label Oct 13, 2021
@chris--jones
Copy link
Contributor Author

Fixed package-lock

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

Awesome! thank you!

@thornjad thornjad merged commit 6a360ab into http-party:master Oct 15, 2021
@thornjad thornjad mentioned this pull request Oct 15, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file patch version Small, non-breaking, bug fix or trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants