Switch from JSHint to ESLint

[Hirse]

• Remove JSHint

• Add ESLint

  • Add prettier plugin
  • Add node plugin
  • Add mocha plugin

• Configure linting rules specific to folder using .eslintrc.json files

  • clicktest: mocha
  • components: browser
  • public/source: browser
  • source: node
  • test: mocha

  Using files in the folders allows editor plugins to get the correct config when a file is opened

• Use ESLint to also check formatting
  With the prettier plugin, we can check for linting issues and formatting with one command

  • Removed check-prettier-formatting grunt-task and added npm run lint script
  • Add lint step to CI builds explicitly

• Fix reported errors

• Use recommended configuration (of ESLint and the plugins) with the following exceptions

  • no-unused-vars: Allow unused parameter as that seems to be current code style
  • no-process-exit: Currently used intentionally
  • no-control-regex: We have to use control characters as they appear in messages from git
  • no-prototype-builtins: Only one occurrence that we can refactor later
  • mocha/no-mocha-arrows: Mocha discourages the use of arrow functions, which are used in the code base
  • mocha/no-setup-in-describe: Can be moved to beforeAll blocks later
  • mocha/no-identical-title: Consider whether current naming is optimal

  The current configuration is aimed at providing the most value without requiring big refactorings for now. Future rules to consider might be:

  • no-var: Require const or let
  • ...

[campersau]

When I run npm run build I get this message:

Running "browserify-components" task
>> ./components/.eslintrc.json/.eslintrc.json.js does not exist. If this component is obsolete, please remove that directory or perform a clean build.

You can also see it in the CI builds.

This is because we use readdir and assume that inside the components folder are only other folders and no files. So we need to change the logic a little bit.

ungit/Gruntfile.js

Lines 154 to 163 in 2a09c95

	fs.readdirSync('components').map((component) => {
	return new Promise((resolve, reject) => {
	const src = `./components/${component}/${component}.js`;
	if (!fs.existsSync(src)) {
	grunt.log.warn(
	`${src} does not exist. If this component is obsolete, please remove that directory or perform a clean build.`
	);
	resolve();
	return;
	}

---

Before we did run jshint as part of build now we have to manually run npm run lint. So we should either:

• Run npm run lint && grunt on build or
• Document that npm run lint must be run manually in the CONTRIBUTING.md

---

Also we now have two options to fix formatting errors:

• npm run format
• npm run lint -- --fix

Do we need both?
If yes should be change format to run eslint . bin/* --fix instead? Should we rename it to e.g. lint-fix?

---

This will be the output when there are linting errors:

PS P:\ungit> npm run lint

> ungit@1.5.7 lint P:\ungit
> eslint . bin/*


P:\ungit\Gruntfile.js
  350:5  error  Insert `;`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! ungit@1.5.7 lint: `eslint . bin/*`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the ungit@1.5.7 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

[Hirse]

@campersau thanks for the detailed review.

• Updated the grunt task to filter out files in the components directory.
• Added documentation to CONTRIBUTING.md
• Changed npm run format to run npm run lint -- --fix

I had removed the linting from the build step to speed up build time and to not stop the build when there are formatting issues.
The linting is only really required before commit. During development linting or formatting issues should not stop the build.

We could consider adding the linting as pre-commit hook or adding it to the build in a non-blocking way (i.e. only issue warnings).

[campersau]

Something I forgot to ask: why is this needed?

[Hirse]

fs.promises was first added in 11.14.0 and later backported to 10.17.0.
We are fine to use it if we support latest Node 10 and latest Node 11.
>= 10.18 would technically include 11.0.0 which does not support fs.promises yet.

^10.18.0 || >=11.14.0 means any version of node 10 starting with 10.8.0 or any version after 11.14.0.

See eslint-plugin-node rule.